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

crd: remove field transformation struct and its usages #46

Merged
merged 4 commits into from
Aug 9, 2022
Merged

crd: remove field transformation struct and its usages #46

merged 4 commits into from
Aug 9, 2022

Conversation

muvaf
Copy link
Member

@muvaf muvaf commented Jul 28, 2022

Description of your changes

FieldTransformation is the one and only struct passed as result from one generator to the other. As mentioned in #9 and #4 (review) , the information it conveys is already available. So, this PR removes it and gathers that information from config.Resource, using it as the source of truth for such info like all other mechanisms.

Fixes #9

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

Only the following diff is reported in the examples in main of provider-aws:
image

@muvaf
Copy link
Member Author

muvaf commented Jul 28, 2022

I separated reference field name calculation into its own PR because it seems like this PR will take a while to merge since it touches very similar places #15 touches.

@muvaf muvaf changed the title crd: remove field transformation struct and its usages and fix reference field name calculations crd: remove field transformation struct and its usages Jul 28, 2022
Copy link
Member

@sergenyalcin sergenyalcin left a comment

Choose a reason for hiding this comment

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

LGTM! @muvaf It seems that this PR touches some places that are related the example generation. I have not very much context on this. So maybe @ulucinar's review on this part can be more accurate. But, for unblocking the apigateway2 PR, I approve this. Thank you very much.

@muvaf
Copy link
Member Author

muvaf commented Jul 28, 2022

@sergenyalcin thanks! I think it's not a risky change but there are conflicts with other example gen PR so I'll wait that before merging. #48 is the blocking PR actually.

@sergenyalcin
Copy link
Member

@sergenyalcin thanks! I think it's not a risky change but there are conflicts with other example gen PR so I'll wait that before merging. #48 is the blocking PR actually.

@muvaf Got it! I realized that the PR was separated after I started the review :) I will also review the blocking PR. #48

…commonly by different generators

Signed-off-by: Muvaffak Onus <[email protected]>
…between crd generator and example generator

Signed-off-by: Muvaffak Onus <[email protected]>
…case where there is no schema in given path

Signed-off-by: Muvaffak Onus <[email protected]>
@muvaf muvaf requested a review from sergenyalcin August 9, 2022 10:32
Copy link
Member

@sergenyalcin sergenyalcin left a comment

Choose a reason for hiding this comment

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

Thanks @muvaf I did a code review and as far as I understand the most important file is example.go In another files, mainly, changes are related the changed function call. This PR looks good to me generally.

As I said that before, these changes are related with your before works together with Alper. So apart from high-level architectural part, I focused on the code review.

@muvaf muvaf merged commit ac4752e into crossplane:main Aug 9, 2022
@muvaf muvaf deleted the field-trans branch August 9, 2022 11:32
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.

Terraform Resource Schema Traversal shared for multiple pipelines
2 participants