Skip to content

Transfer tgc/cai2hcl ownership to magic-modules#8824

Merged
melinath merged 11 commits into
GoogleCloudPlatform:mainfrom
amirkaromashkin:feature/tgc_cai2hcl
Sep 26, 2023
Merged

Transfer tgc/cai2hcl ownership to magic-modules#8824
melinath merged 11 commits into
GoogleCloudPlatform:mainfrom
amirkaromashkin:feature/tgc_cai2hcl

Conversation

@amirkaromashkin

@amirkaromashkin amirkaromashkin commented Aug 31, 2023

Copy link
Copy Markdown
Contributor

This change moves tgc/cai2hcl management to magic-modules/mmv1 using new provider, which will then be used for generated cai2hcl converters.

terraform-google-conversion/cai2hcl folder is now managed from GCP/magic-modules

This change moves tgc/cai2hcl management to magic-modules/mmv1
using new provider, which will then be used for generated
cai2hcl converters.
@modular-magician

This comment was marked as outdated.

@amirkaromashkin amirkaromashkin changed the title Transfer tgc/cai2hcl to magic-modules. Transfer tgc/cai2hcl to magic-modules Aug 31, 2023
@modular-magician

Copy link
Copy Markdown
Collaborator

Hello! I am a robot. It looks like you are a: Community Contributor Googler Core Contributor. Tests will run automatically.

@roaks3, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look.

You can help make sure that review is quick by doing a self-review and by running impacted tests locally.

@modular-magician

Copy link
Copy Markdown
Collaborator

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

Diff report

Your PR hasn't generated any diffs, but I'll let you know if a future commit does.

@modular-magician

This comment was marked as outdated.

@amirkaromashkin amirkaromashkin changed the title Transfer tgc/cai2hcl to magic-modules Transfer tgc/cai2hcl ownership to magic-modules Aug 31, 2023
@amirkaromashkin amirkaromashkin marked this pull request as ready for review August 31, 2023 18:51
@modular-magician

Copy link
Copy Markdown
Collaborator

Tests analytics

Total tests: 3008
Passed tests 2712
Skipped tests: 296
Affected tests: 0

$\textcolor{green}{\textsf{All tests passed in REPLAYING mode.}}$
View the build log

- Add missing -a param
- Move cai2hcl folder name from ruby files to bash
Remove files in cai2hcl/ folder instead of a folder itself.
@modular-magician

Copy link
Copy Markdown
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 ( 24 files changed, 801 insertions(+), 272 deletions(-))

@modular-magician

Copy link
Copy Markdown
Collaborator

Tests analytics

Total tests: 3008
Passed tests 2709
Skipped tests: 296
Affected tests: 3

Action taken

Found 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccDataprocJobIamPolicy|TestAccHealthcareDatasetIamPolicy|TestAccSpannerDatabaseIamPolicy

Get to know how VCR tests work

@modular-magician

Copy link
Copy Markdown
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccDataprocJobIamPolicy[Debug log]
TestAccHealthcareDatasetIamPolicy[Debug log]
TestAccSpannerDatabaseIamPolicy[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$


$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

@roaks3

roaks3 commented Sep 6, 2023

Copy link
Copy Markdown
Contributor

@amirkaromashkin I'm a little surprised by this change, but I may be missing context. Do you have an internal issue tracking this work, or have you been working with a member of our team on this?

@melinath

melinath commented Sep 7, 2023

Copy link
Copy Markdown
Member

@roaks3 I've been talking with Amir about this change - this PR should make MM take over management of cai2hcl with no diffs - other changes would ideally be in other PRs (but I believe some of these changes were necessary here as part of bringing the files under managment.)

@roaks3 roaks3 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ok, got it, and I reviewed the internal doc. I took a first pass and things looked good, but to help with review, am I correct in assuming that mmv1/third_party/cai2hcl files are copied verbatim from somewhere?

Comment thread mmv1/provider/terraform_tgc_cai2hcl.rb Outdated
@amirkaromashkin

Copy link
Copy Markdown
Contributor Author

Ok, got it, and I reviewed the internal doc. I took a first pass and things looked good, but to help with review, am I correct in assuming that mmv1/third_party/cai2hcl files are copied verbatim from somewhere?

Its not completely verbatim. Its a slightly refactored version of https://github.com/GoogleCloudPlatform/terraform-google-conversion/tree/main/cai2hcl: converters themselves were not changed, I changed packages/folder structure and helper methods like convert.go.

Testdata files copied from https://github.com/GoogleCloudPlatform/terraform-google-conversion/tree/main/testdata as is.

It is not yet needed at this stage for manual converters.
@modular-magician

Copy link
Copy Markdown
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 ( 23 files changed, 709 insertions(+), 219 deletions(-))

@edisonhuang-google

Copy link
Copy Markdown
Contributor

LGTM

Otherwise it breaks current Convert API.
@modular-magician

Copy link
Copy Markdown
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, 667 insertions(+), 177 deletions(-))

@modular-magician

Copy link
Copy Markdown
Collaborator

Tests analytics

Total tests: 3020
Passed tests 2723
Skipped tests: 297
Affected tests: 0

$\textcolor{green}{\textsf{All tests passed in REPLAYING mode.}}$
View the build log

@roaks3

roaks3 commented Sep 11, 2023

Copy link
Copy Markdown
Contributor

I just tried to take another look here and this is very hard to review with the assumption that everything is newly written (you mentioned that some refactoring happened). Could you please provide some more context on what was changed (and perhaps why)? Where should I be looking for changes to the previous behavior vs. parts that are copy-and-pasted?

@amirkaromashkin

Copy link
Copy Markdown
Contributor Author

I just tried to take another look here and this is very hard to review with the assumption that everything is newly written (you mentioned that some refactoring happened). Could you please provide some more context on what was changed (and perhaps why)? Where should I be looking for changes to the previous behavior vs. parts that are copy-and-pasted?

I guess there are 2 ways to handle it:

  1. I give the broad explanation of all the changes and refactoring I made to the https://github.com/GoogleCloudPlatform/terraform-google-conversion/tree/main/cai2hcl
  2. I first manually update the target repo, get it merged and then this migration will be verbatim (I'll need to give explanations there, but this will be less complicated)

I guess the 2nd option looks better. WDYT? @roaks3

"converters/google/resources", same as in tfplan2cai
@amirkaromashkin

Copy link
Copy Markdown
Contributor Author

I just tried to take another look here and this is very hard to review with the assumption that everything is newly written (you mentioned that some refactoring happened). Could you please provide some more context on what was changed (and perhaps why)? Where should I be looking for changes to the previous behavior vs. parts that are copy-and-pasted?

I guess there are 2 ways to handle it:

  1. I give the broad explanation of all the changes and refactoring I made to the https://github.com/GoogleCloudPlatform/terraform-google-conversion/tree/main/cai2hcl
  2. I first manually update the target repo, get it merged and then this migration will be verbatim (I'll need to give explanations there, but this will be less complicated)

I guess the 2nd option looks better. WDYT? @roaks3

I have just found a slight bug in a folder structure when started to implement the Option2. So I believe its a better approach. On it.

@modular-magician

Copy link
Copy Markdown
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, 667 insertions(+), 177 deletions(-))

@roaks3

roaks3 commented Sep 12, 2023

Copy link
Copy Markdown
Contributor

Yea, I think the second option would be ideal here. Thanks!

@roaks3

roaks3 commented Sep 12, 2023

Copy link
Copy Markdown
Contributor

Passing over to @melinath as I will likely be on PTO when this is ready for another look

@roaks3 roaks3 requested review from melinath and removed request for roaks3 September 12, 2023 19:36
@melinath

Copy link
Copy Markdown
Member

GoogleCloudPlatform/terraform-google-conversion#1416 is the PR to refactor TGC first.

@melinath melinath left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Refactor of TGC is complete - marking this as reviewed pending updates to match the final structure we settled on.

Once refactoring of terraform-google-conversion is
merged, we we update cai2hcl/ folder so that this PR
moves functionality from tgc to mmv1 verbatim without
any refactorings.
@amirkaromashkin

Copy link
Copy Markdown
Contributor Author

Refactor of TGC is complete - marking this as reviewed pending updates to match the final structure we settled on.

Done. Now it should produce zero diff

@modular-magician

Copy link
Copy Markdown
Collaborator

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

Diff report

Your PR hasn't generated any diffs, but I'll let you know if a future commit does.

@modular-magician

Copy link
Copy Markdown
Collaborator

Tests analytics

Total tests: 3077
Passed tests 2778
Skipped tests: 299
Affected tests: 0

$\textcolor{green}{\textsf{All tests passed in REPLAYING mode.}}$
View the build log

@melinath melinath left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@melinath melinath merged commit ef1cca7 into GoogleCloudPlatform:main Sep 26, 2023
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