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

Cai2Hcl: refactor of converter_map and split of utils.go #9378

Conversation

amirkaromashkin
Copy link
Contributor

@amirkaromashkin amirkaromashkin commented Oct 28, 2023

Refactor converter_map.go to converter.go, which implements Converter interface.

This PR does not change any logic.

These are: BackendService, GlobalBackendService, ForwardingRule.

This is the output of cai2hcl provider, commited as is
without the generator Ruby code itself, as agreed with
library owners.
Reason: limited capacity of the supporting team to
perform code reviews and the future need to port this
solution to golang as a part of migration to go. As soon
the staffing and/or migration is done, we will come back
to implement the generated converters as
originally planned.
@amirkaromashkin amirkaromashkin changed the title Add cai2hcl generated converters for 3 Compute resources. Add cai2hcl generated converters for 3 Compute resources Oct 28, 2023
@amirkaromashkin amirkaromashkin marked this pull request as ready for review November 1, 2023 23:04
@rileykarson rileykarson self-requested a review November 2, 2023 16:10
@modular-magician

This comment was marked as outdated.

@rileykarson

This comment was marked as outdated.

Copy link
Member

@melinath melinath left a comment

Choose a reason for hiding this comment

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

This is a pretty massive set of changes - could you split it up into multiple PRs? I'd suggest at a minimum: one PR for the converter / shared files refactor and one for each resource. That should make the overall review go faster. It would probably also be useful to provide some context about why the changes to the converter / shared files are required.

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

TF Conversion: Diff ( 35 files changed, 4031 insertions(+), 377 deletions(-))

1. Refactor of shared files (Uber Converter)
2. Add ForwardingRuleConverter (generated)
3. Add BackendServiceConverters (generated)
+ ability to match converters by name

This commit removes part2 and part3, leaving only part1 in this branch.
@amirkaromashkin amirkaromashkin changed the title Add cai2hcl generated converters for 3 Compute resources Cai2Hcl: refactoring ConverterMap from convention to interface Nov 19, 2023
@modular-magician

This comment was marked as outdated.

@amirkaromashkin amirkaromashkin changed the title Cai2Hcl: refactoring ConverterMap from convention to interface Cai2Hcl: refactor ConverterMap to UberConverter interface Nov 19, 2023
@amirkaromashkin amirkaromashkin changed the title Cai2Hcl: refactor ConverterMap to UberConverter interface Cai2Hcl: refactor converter_map.go interface and split utils.go Nov 19, 2023
@amirkaromashkin amirkaromashkin changed the title Cai2Hcl: refactor converter_map.go interface and split utils.go Cai2Hcl: refactor converter_map interface and split utils Nov 19, 2023
@amirkaromashkin
Copy link
Contributor Author

This is a pretty massive set of changes - could you split it up into multiple PRs? I'd suggest at a minimum: one PR for the converter / shared files refactor and one for each resource. That should make the overall review go faster. It would probably also be useful to provide some context about why the changes to the converter / shared files are required.

Right, it was not that big of a PR when Ruby generator was there, but in any case you're right, too many logical changes. It can be split into 3 chunks:

  1. Refactor of converter_map and split of utils.go
  2. Manually add ForwardingRule generated converter
  3. Ability to match converters by ResourceName + Manually add BackendService+GlobalBackendService generated converter

So I have updated this PR to Part1: Refactor of converter_map and split of utils.go.
PTAL. Thanks!

@amirkaromashkin amirkaromashkin changed the title Cai2Hcl: refactor converter_map interface and split utils Cai2Hcl: refactor of converter_map and split of utils.go Nov 19, 2023
@modular-magician

This comment was marked as outdated.

1 similar comment
@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

TF Conversion: Diff ( 22 files changed, 405 insertions(+), 256 deletions(-))

@rileykarson
Copy link
Member

@melinath do you mind picking up review? I've been unable to fit this in since I'll need to onboard myself to TGC enough to review in the process.

@melinath
Copy link
Member

melinath commented Dec 4, 2023

yep, can do!

mmv1/third_party/cai2hcl/converter_test.go Outdated Show resolved Hide resolved
mmv1/third_party/cai2hcl/common/utils.go Outdated Show resolved Hide resolved
)

// Converter which aggregates all service-specific converters in the same interface.
type UberConverter 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 would be interested in potentially calling this Converter and having a NewConverter function, so that the implementation more closely matches tfplan2cai: https://github.com/GoogleCloudPlatform/terraform-google-conversion/blob/main/tfplan2cai/converters/google/convert.go

That said, I'm also open to alternatives - mostly I think it would be good to converge on a more similar implementation between the two halves over time so that we can reduce cognitive load moving between them.

What would you think?

Copy link
Contributor Author

@amirkaromashkin amirkaromashkin Dec 8, 2023

Choose a reason for hiding this comment

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

In Cai2hcl Converter is an interface, so we cannot call it so.

UberConverter is on one hand is a Converter (implements this interface and used as one by unit tests), but on the other hand - its a new interface for a group of converters, which is frustrating, I agree.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the interface would need to be renamed as well - or we would need to stop using interfaces like this. Overall I'm concerned that there's a lot of complexity / nesting going on that ends up obfuscating what's happening, and I'm not sure it's benefitting us. But I could totally be forgetting or not considering something!

FWIW my inclination would be that we standardize towards how tfplan2cai works (having a ResourceConverter struct with a predictable structure - this is similar to how Terraform resources are implemented.

This would basically mean replacing the New*Converter functions with a ResourceConverter that returns a standard type which knows the Terraform type it converts to.

We could than have a single map that of all asset types to relevant resource converters, similar to resource_converters.go in tfplan2cai and provider_mmv1_resources.go in the provider. It's not elegant but it's very clear what's going on, and we would be able to avoid the indirection of the ConverterMap (and I think the names of things could be clearer as well.)

I have a vague memory that there was a reason we didn't do this previously - possibly a circular dependency? - but I don't remember why it was a problem for cai2hcl and not for tfplan2cai. And that might have been related to a completely different decision.

As before, I'm open to alternatives (and even hypothetically keeping the current structure) if there's a clear benefit or a blocker of some kind that outweighs standardizing on our other libraries.

mmv1/third_party/cai2hcl/common/hclwrite.go Outdated Show resolved Hide resolved
mmv1/third_party/cai2hcl/common/uber_converter.go Outdated Show resolved Hide resolved
@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

TF Conversion: Diff ( 22 files changed, 386 insertions(+), 257 deletions(-))

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

TF Conversion: Diff ( 21 files changed, 316 insertions(+), 199 deletions(-))

@melinath
Copy link
Member

melinath commented Dec 8, 2023

Going through and reminding myself of some background and noting it here for myself / posterity. This PR is a follow-up to GoogleCloudPlatform/terraform-google-conversion#1416

Copy link
Member

@melinath melinath left a comment

Choose a reason for hiding this comment

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

I know that you've been waiting for review on this for a while and I'm sorry about the delay and that I'm asking about refactoring further - but since you're proposing changes and we're moving from "proof of concept" towards what we expect to have in place longer-term it would be great to be moving towards what might be a more final design.

mmv1/third_party/cai2hcl/common/hcl_write.go Show resolved Hide resolved
)

// Converter which aggregates all service-specific converters in the same interface.
type UberConverter struct {
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the interface would need to be renamed as well - or we would need to stop using interfaces like this. Overall I'm concerned that there's a lot of complexity / nesting going on that ends up obfuscating what's happening, and I'm not sure it's benefitting us. But I could totally be forgetting or not considering something!

FWIW my inclination would be that we standardize towards how tfplan2cai works (having a ResourceConverter struct with a predictable structure - this is similar to how Terraform resources are implemented.

This would basically mean replacing the New*Converter functions with a ResourceConverter that returns a standard type which knows the Terraform type it converts to.

We could than have a single map that of all asset types to relevant resource converters, similar to resource_converters.go in tfplan2cai and provider_mmv1_resources.go in the provider. It's not elegant but it's very clear what's going on, and we would be able to avoid the indirection of the ConverterMap (and I think the names of things could be clearer as well.)

I have a vague memory that there was a reason we didn't do this previously - possibly a circular dependency? - but I don't remember why it was a problem for cai2hcl and not for tfplan2cai. And that might have been related to a completely different decision.

As before, I'm open to alternatives (and even hypothetically keeping the current structure) if there's a clear benefit or a blocker of some kind that outweighs standardizing on our other libraries.

mmv1/third_party/cai2hcl/common/utils.go Outdated Show resolved Hide resolved
@melinath
Copy link
Member

melinath commented Dec 8, 2023

@iyabchen since you did the initial implementation it would be great if you could take a look and add any relevant context that you might remember from that

@melinath melinath requested a review from iyabchen December 8, 2023 21:29
@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

TF Conversion: Diff ( 21 files changed, 256 insertions(+), 199 deletions(-))

@amirkaromashkin
Copy link
Contributor Author

Following this thread: #9378 (comment) (not sure why I cant just respond to it directly).

There are 2 topics, lets discuss them separately.

  1. ConverterMap location (single file of 1 file per API folder):
    Your proposal is to have a single ConverterMap file for all converters, not a file for per feature area.
    One of the issues I see is indeed a cycle dep sue to unit tests, which are in cai2chl located near the converters in cai2hcl, in tfplan2cai - outside. This would create a cycle cai2hcl -(converter ref)-> compute -(converterMap ref from unit test)-> cai2hcl. Still I can try to overcome somehow (by extracting some new package maybe).

  2. ConverterMap structure.
    tfplan2cai is a mapping from string to ResourceConverter. But in Cai2hcl, as I mentioned in comments above (here) there is a different chain: Asset Type -> matchingStrategy -> ResourceConverter.
    Its because I.e. compute_instance_template and compute_region_instance_template they have the same Asset Type: InstanceTemplate (CAI Docs).
    Same applies to InstanceGroup, ForwardingRule, BackendService and many other Compute types.
    For FR and BS its not 100% so because CAI supported types list this "artificial" types like GlobalForwardingRule and RegionBackendService, still it needs additional effort to set these types in our codebase., Also - its officially not the case InstanceGroup and InstanceTemplate, so I would prefer to not mandate this differentiator from library clients and use a self link matcher.
    We can still refer to this file as converter_map.go though, but the value should not be the converter, but smth else.
    What I mean through all of this, is that ConverterMap is more complicated in cai2hcl than in tfplan2cai and we should not require too much similarity to their interface/structure.
    Also that being said I'm not so sure that 1 file will work considering the increased complexity of the mapping.

So to sum up - I will try to:

  1. Have 1 converter_map file totally
  2. Stick to the converter_map pattern, while keeping the converter matching flexibility (by selflink pattern)

1. Rename UberConverter to ConverterMap.
2. Remove ConverterMap's from each API folder and replace them with a single top-level ConverterMap
@amirkaromashkin
Copy link
Contributor Author

Following this thread: #9378 (comment) (not sure why I cant just respond to it directly).

There are 2 topics, lets discuss them separately.

  1. ConverterMap location (single file of 1 file per API folder):
    Your proposal is to have a single ConverterMap file for all converters, not a file for per feature area.
    One of the issues I see is indeed a cycle dep sue to unit tests, which are in cai2chl located near the converters in cai2hcl, in tfplan2cai - outside. This would create a cycle cai2hcl -(converter ref)-> compute -(converterMap ref from unit test)-> cai2hcl. Still I can try to overcome somehow (by extracting some new package maybe).
  2. ConverterMap structure.
    tfplan2cai is a mapping from string to ResourceConverter. But in Cai2hcl, as I mentioned in comments above (here) there is a different chain: Asset Type -> matchingStrategy -> ResourceConverter.
    Its because I.e. compute_instance_template and compute_region_instance_template they have the same Asset Type: InstanceTemplate (CAI Docs).
    Same applies to InstanceGroup, ForwardingRule, BackendService and many other Compute types.
    For FR and BS its not 100% so because CAI supported types list this "artificial" types like GlobalForwardingRule and RegionBackendService, still it needs additional effort to set these types in our codebase., Also - its officially not the case InstanceGroup and InstanceTemplate, so I would prefer to not mandate this differentiator from library clients and use a self link matcher.
    We can still refer to this file as converter_map.go though, but the value should not be the converter, but smth else.
    What I mean through all of this, is that ConverterMap is more complicated in cai2hcl than in tfplan2cai and we should not require too much similarity to their interface/structure.
    Also that being said I'm not so sure that 1 file will work considering the increased complexity of the mapping.

So to sum up - I will try to:

  1. Have 1 converter_map file totally
  2. Stick to the converter_map pattern, while keeping the converter matching flexibility (by selflink pattern)

Done.

  1. Renamed UberConverter to ConverterMap.
  2. Removed ConverterMap's from each API folder and replace them with a single top-level ConverterMap
  3. Changed unit tests packages to "<API_NAME>_test" to avoid dependency cycle.

Some visible potential issues are:

  1. Unit tests packages are different then the folder name.
  2. ConverterMap is not actually a "map", but a configuration object with maps (now 2 maps, in future - another Matchers map will come).
    IMO, these compromises are ok.

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

TF Conversion: Diff ( 16 files changed, 159 insertions(+), 172 deletions(-))

Copy link
Member

@melinath melinath left a comment

Choose a reason for hiding this comment

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

(The reason you couldn't reply to the comment was because it was technically a comment on an existing thread, and GitHub threading is weird and confusing and will only let you comment on the main copy of a particular thread.)

Using *_test packages should be fine. 👍

I put down some more thoughts about the structure here - let me know what you think!

mmv1/third_party/cai2hcl/common/converter_map.go Outdated Show resolved Hide resolved
@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

TF Conversion: Diff ( 19 files changed, 153 insertions(+), 197 deletions(-))

Copy link
Member

@melinath melinath left a comment

Choose a reason for hiding this comment

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

Overall this is looking good. I'm still concerned about the maintenance burden / cognitive load of multiple maps for converters; I have a suggestion that I think should address it - let me know what you think!

mmv1/third_party/cai2hcl/convert.go Outdated Show resolved Hide resolved
@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

TF Conversion: Diff ( 18 files changed, 142 insertions(+), 196 deletions(-))

Copy link
Member

@melinath melinath left a comment

Choose a reason for hiding this comment

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

I'm not entirely satisfied with the converter map structure but I may be overthinking it. This version is much easier to understand than what we had before and should be fairly easy to refactor in the future if needed (and we might know more than we do now.) This LGTM.

@melinath melinath merged commit ddc054c into GoogleCloudPlatform:main Jan 3, 2024
13 checks passed
kylase pushed a commit to yuanchuankee/magic-modules that referenced this pull request Jan 21, 2024
…Platform#9378)

* Add cai2hcl generated converters for 3 Compute resources.

These are: BackendService, GlobalBackendService, ForwardingRule.

This is the output of cai2hcl provider, commited as is
without the generator Ruby code itself, as agreed with
library owners.
Reason: limited capacity of the supporting team to
perform code reviews and the future need to port this
solution to golang as a part of migration to go. As soon
the staffing and/or migration is done, we will come back
to implement the generated converters as
originally planned.

* Split current PR into 3 chunks:
1. Refactor of shared files (Uber Converter)
2. Add ForwardingRuleConverter (generated)
3. Add BackendServiceConverters (generated)
+ ability to match converters by name

This commit removes part2 and part3, leaving only part1 in this branch.

* Fix versions after merge conflict

* Remove auto-generated code header from FR

* Remove unused util function

* Fix merge conflict resolution issue

* Remove unnecessary result map normalization (will needed later with generated converters)

* Simplify cai2hcl by reworking UberConverter's as a single ConverterMap.

1. Rename UberConverter to ConverterMap.
2. Remove ConverterMap's from each API folder and replace them with a single top-level ConverterMap

* Simplify converters

* Flatten converter_map.go and use strings as converter names instead of constants.
balanaguharsha pushed a commit to balanaguharsha/magic-modules that referenced this pull request May 2, 2024
…Platform#9378)

* Add cai2hcl generated converters for 3 Compute resources.

These are: BackendService, GlobalBackendService, ForwardingRule.

This is the output of cai2hcl provider, commited as is
without the generator Ruby code itself, as agreed with
library owners.
Reason: limited capacity of the supporting team to
perform code reviews and the future need to port this
solution to golang as a part of migration to go. As soon
the staffing and/or migration is done, we will come back
to implement the generated converters as
originally planned.

* Split current PR into 3 chunks:
1. Refactor of shared files (Uber Converter)
2. Add ForwardingRuleConverter (generated)
3. Add BackendServiceConverters (generated)
+ ability to match converters by name

This commit removes part2 and part3, leaving only part1 in this branch.

* Fix versions after merge conflict

* Remove auto-generated code header from FR

* Remove unused util function

* Fix merge conflict resolution issue

* Remove unnecessary result map normalization (will needed later with generated converters)

* Simplify cai2hcl by reworking UberConverter's as a single ConverterMap.

1. Rename UberConverter to ConverterMap.
2. Remove ConverterMap's from each API folder and replace them with a single top-level ConverterMap

* Simplify converters

* Flatten converter_map.go and use strings as converter names instead of constants.
pengq-google pushed a commit to pengq-google/magic-modules that referenced this pull request May 21, 2024
…Platform#9378)

* Add cai2hcl generated converters for 3 Compute resources.

These are: BackendService, GlobalBackendService, ForwardingRule.

This is the output of cai2hcl provider, commited as is
without the generator Ruby code itself, as agreed with
library owners.
Reason: limited capacity of the supporting team to
perform code reviews and the future need to port this
solution to golang as a part of migration to go. As soon
the staffing and/or migration is done, we will come back
to implement the generated converters as
originally planned.

* Split current PR into 3 chunks:
1. Refactor of shared files (Uber Converter)
2. Add ForwardingRuleConverter (generated)
3. Add BackendServiceConverters (generated)
+ ability to match converters by name

This commit removes part2 and part3, leaving only part1 in this branch.

* Fix versions after merge conflict

* Remove auto-generated code header from FR

* Remove unused util function

* Fix merge conflict resolution issue

* Remove unnecessary result map normalization (will needed later with generated converters)

* Simplify cai2hcl by reworking UberConverter's as a single ConverterMap.

1. Rename UberConverter to ConverterMap.
2. Remove ConverterMap's from each API folder and replace them with a single top-level ConverterMap

* Simplify converters

* Flatten converter_map.go and use strings as converter names instead of constants.
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.

5 participants