Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add configuration API parameter for overriding the field names #335

Merged

Conversation

sergenyalcin
Copy link
Member

Description of your changes

This PR adds configuration API parameter for overriding the field names.

OverrideFieldNames allows to manually override the relevant field name to avoid possible Go struct name conflicts that may occur after Multiversion CRDs support. During field generation, there may be fields with the same struct name calculated in the same group. For example, let X and Y resources in the same API group have a field named Tag. This field is an object type and the name calculated for the struct to be generated is TagParameters (for spec) for both resources. To avoid this conflict, upjet looks at all previously created structs in the package during generation and if there is a conflict, it puts the Kind name of the related resource in front of the next one: YTagParameters. With Multiversion CRDs support, the above conflict scenario cannot be solved in the generator when the old API group is preserved and not regenerated, because the generator does not know the object names in the old version. For example, a new API version is generated for resource X. In this case, no generation is done for the old version of X and when Y is generated, the generator is not aware of the TagParameters in X and generates TagParameters instead of YTagParameters. Thus, two object types with the same name are generated in the same package. This can be overcome by using this configuration API. The key of the map indicates the name of the field that is generated and causes the conflict, while the value indicates the name used to avoid the conflict. By convention, also used in upjet, the field name is preceded by the value of the generated Kind, for example: "TagParameters": "ClusterTagParameters"

I have:

  • Read and followed Upjet'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

Tested against provider-aws autoscaling.AutoscalingGroup and autoscaling.GroupTag resources

Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
Copy link
Collaborator

@ulucinar ulucinar left a comment

Choose a reason for hiding this comment

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

Thanks @sergenyalcin, lgtm.

I think there's need to make some architectural changes in upjet's code generation pipelines. We will need to aim generators that can analyze the existing code and generate code accordingly. This should result in more powerful generators that can be run independently from the other generators. Some of the current generators depend on the running of some previous generators and passing context (type & parameter information, etc.) in memory. We should consider isolating these generators so that we can independently run them in our code generation pipelines.

Comment on lines 452 to 454
if v, ok := overrideFieldNames[n]; ok {
return v, nil
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: So, my understanding is that we would like to override any auto-generated name that we calculate here with the manually configured (overridden) name. If this is the case, then shouldn't we override what's calculated at the end of this function because there are some further modifications down the path in this function. The idea is:

  • We bump an API version
  • Run make generate and observe a parameter name conflict
  • At this point, it would be easier for us to use the exact conflicting name as the key.

If we do the look up early here, then the override key may not match the actual conflicting name. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense. This implementation resolves the problems until we observed. However, your suggestion is more powerful and generic. Thanks @ulucinar!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you @sergenyalcin.

@@ -180,7 +180,7 @@ func TestBuilder_generateTypeName(t *testing.T) {
g := &Builder{
Package: p,
}
got, gotErr := generateTypeName(tc.args.suffix, g.Package, tc.args.names...)
got, gotErr := generateTypeName(tc.args.suffix, g.Package, map[string]string{}, tc.args.names...)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please add a test case checking the override functionality?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
@sergenyalcin sergenyalcin merged commit 62169a1 into crossplane:main Jan 30, 2024
7 checks passed
@sergenyalcin sergenyalcin deleted the add-override-fieldnames-api branch January 30, 2024 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants