Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Find a unique type name by appending indexes #137

Merged
merged 4 commits into from
Nov 13, 2021

Conversation

turkenh
Copy link
Member

@turkenh turkenh commented Nov 10, 2021

Description of your changes

Fixes #135

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

With provider-tf-gcp by generating all resources.

pkg/config/provider.go Outdated Show resolved Hide resolved
@turkenh turkenh requested review from muvaf and ulucinar November 10, 2021 13:34
pkg/pipeline/crd.go Outdated Show resolved Hide resolved
pkg/types/builder.go Outdated Show resolved Hide resolved
@@ -293,6 +293,14 @@ func (g *Builder) generateTypeName(suffix string, names ...string) (string, erro
if g.Package.Scope().Lookup(n) == nil {
return n, nil
}
// start from 2 considering the 1st of this type is the one without an
// index.
for i := 2; i < 10; i++ {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can get away with appending the CRD kind name repeatedly. Instead of SubnetworkParameters2, how would SubnetworkParameters_Subnetwork or Subnetwork_SubnetworkParameters look? Feels like because they are in the same package, it's kind of too easy to confuse the one used by one resource with the other when it's only a number that differentiates them.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given this is just generated code and we don't expect anyone to deal with modifying those types, I prefer numbers.
One case could be people importing these types to interact programmatically but still, in that case, I believe using the wrong type would cause compilation errors hence easy to figure out.

Even if we go with appending the kind name as you suggested, we will still have both SubnetworkParameters and SubnetworkParameters_Subnetwork in the same package and the former would belong to RouterNat resource instead of Subnetwork which is still confusing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about someone importing this package and using the types rather than modifying. In such cases, the types being generated or not doesn't really matter for them and Go types having numbers in the name doesn't look very idiomatic. I'm OK with both approaches though.

@turkenh turkenh force-pushed the fix-typename-conflict branch from 3fa99a6 to e539c45 Compare November 11, 2021 12:09
@turkenh turkenh requested a review from muvaf November 11, 2021 12:10
@turkenh
Copy link
Member Author

turkenh commented Nov 11, 2021

@muvaf your comments got resolved!

pkg/types/builder.go Outdated Show resolved Hide resolved
pkg/types/builder.go Outdated Show resolved Hide resolved
Comment on lines 49 to 48
// Generated stores the information of the generated types
type Generated struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a great suggestion but Generated/types.Generated feels too generic.

types.TypeTree

Suggested change
// Generated stores the information of the generated types
type Generated struct {
// TypeTree contains all the types and their sibling relations.
type TypeTree struct {

types.CustomResourceDefinition

Suggested change
// Generated stores the information of the generated types
type Generated struct {
// CustomResourceDefinition includes all Go types necessary to print a CustomResourceDefinition Go struct.
type CustomResourceDefinition struct {

types.Managed

Suggested change
// Generated stores the information of the generated types
type Generated struct {
// Managed includes all Go types necessary to print the Go structs for a Managed resource.
type Managed struct {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot claim that types.Generated is great but I couldn't find a better alternative and honestly would prefer it to the alternatives you suggested above :)

I believe it is not that bad and actually conveys its purpose: types.Generated is a struct that holds Generated types

Resolves crossplane#135

Signed-off-by: Hasan Turken <turkenh@gmail.com>
Signed-off-by: Hasan Turken <turkenh@gmail.com>
@turkenh turkenh force-pushed the fix-typename-conflict branch 2 times, most recently from 4430812 to 9c12bbb Compare November 11, 2021 20:56
Signed-off-by: Hasan Turken <turkenh@gmail.com>
@turkenh turkenh force-pushed the fix-typename-conflict branch from 9c12bbb to d3450c1 Compare November 11, 2021 20:58
@turkenh turkenh requested a review from muvaf November 11, 2021 20:58
Copy link
Member

@muvaf muvaf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! It'd be great if you can add tests for generateTypeName.

@turkenh
Copy link
Member Author

turkenh commented Nov 12, 2021

LGTM! It'd be great if you can add tests for generateTypeName.

@muvaf indeed I attempted to add but postponed to a larger refactor since I wanted to change the function signature as follows for easier testability:

- func (g *Builder) generateTypeName(suffix string, names ...string) (string, error) {

+ type lookupFunc func(name string) types.Object
+ func generateTypeName(lookup lookupFunc, suffix string, names ...string) (string, error) {

If this sounds ok to you, I can add unit tests with that.

@muvaf
Copy link
Member

muvaf commented Nov 12, 2021

@turkenh you can actually mock the package and its scope like the following without changing the signature: https://github.com/crossplane/crossplane-tools/blob/fa70b84/internal/method/resolver_test.go#L229

Signed-off-by: Hasan Turken <turkenh@gmail.com>
@turkenh
Copy link
Member Author

turkenh commented Nov 13, 2021

@muvaf added unit tests without changing the function signature.

@turkenh turkenh merged commit a0f23e0 into crossplane:main Nov 13, 2021
@turkenh turkenh deleted the fix-typename-conflict branch November 13, 2021 10:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Could not generate a unique name for SubnetworkParameters
2 participants