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

feat: add provider version to metadata generation #2496

Merged
merged 33 commits into from
Sep 9, 2024

Conversation

qz267
Copy link
Contributor

@qz267 qz267 commented Aug 7, 2024

Feature request from: b/356844884

Add ProviderVersion to metadata auto generation. Changes including:

  • Added parseBlueprintProviderVersions function to tfconfig.go
  • Added TestTFProviderVersion to tfconfig_test.go
  • Wired up parseBlueprintProviderVersions to getBlueprintRequirements function, which was called in the metadata generation process.
  • Update golden-metadata.yaml requirements block, added provider versions block.

Checks:

  • make go_test passing
  • make int_test passing
  • make docker_go_lint passing

@qz267 qz267 changed the title [WIP]-356844884-add provider version to metadata generation [b/356844884]-add provider version to metadata generation Aug 9, 2024
@bharathkkb bharathkkb self-assigned this Aug 9, 2024
@bharathkkb bharathkkb changed the title [b/356844884]-add provider version to metadata generation feat: [b/356844884]-add provider version to metadata generation Aug 9, 2024
Copy link
Member

@bharathkkb bharathkkb left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @qz267! Few minor comments

cli/bpmetadata/proto/bpmetadata.proto Outdated Show resolved Hide resolved
cli/bpmetadata/tfconfig.go Outdated Show resolved Hide resolved
cli/bpmetadata/tfconfig_test.go Show resolved Hide resolved
@qz267
Copy link
Contributor Author

qz267 commented Sep 6, 2024

@bharathkkb a friendly ping, all the comments addressed, this PR ready for another review.

cli/bpmetadata/tfconfig.go Outdated Show resolved Hide resolved
cli/bpmetadata/tfconfig.go Outdated Show resolved Hide resolved
cli/bpmetadata/tfconfig.go Outdated Show resolved Hide resolved
cli/bpmetadata/tfconfig.go Show resolved Hide resolved
Copy link
Member

@bharathkkb bharathkkb left a comment

Choose a reason for hiding this comment

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

Thanks for updating, just two last comments

cli/bpmetadata/tfconfig.go Outdated Show resolved Hide resolved
cli/bpmetadata/tfconfig.go Outdated Show resolved Hide resolved
@bharathkkb bharathkkb changed the title feat: [b/356844884]-add provider version to metadata generation feat: add provider version to metadata generation Sep 6, 2024
@bharathkkb bharathkkb enabled auto-merge (squash) September 6, 2024 22:02
@qz267
Copy link
Contributor Author

qz267 commented Sep 9, 2024

hmm, rerun the unit tests on local branch multiple times, all passing, might need to rerun the action job?

@apeabody apeabody disabled auto-merge September 9, 2024 15:27
@apeabody
Copy link
Collaborator

apeabody commented Sep 9, 2024

This is from the CFT CLI Test:

Error! Generated metadata(s) do not match the golden(s).
diff --git a/metadata.yaml b/metadata.yaml
index ffb9656..6059145 100644
--- a/metadata.yaml
+++ b/metadata.yaml
@@ -250,7 +250,7 @@ spec:
       - compute.googleapis.com
       - serviceusage.googleapis.com
     providerVersions:
-      - source: hashicorp/random
-        version: ">= 2.1"
       - source: hashicorp/google
         version: ">= 4.42, < 5.0"
+      - source: hashicorp/random
+        version: ">= 2.1"

Looks like maybe update the line ordering in cli/bpmetadata/int-test/goldens/golden-metadata.yaml?

@qz267
Copy link
Contributor Author

qz267 commented Sep 9, 2024

Updated golden metadata yml provider data.

@apeabody apeabody merged commit 9b4036c into GoogleCloudPlatform:master Sep 9, 2024
14 checks passed
@bharathkkb
Copy link
Member

@qz267 I missed that RequiredProviders output was coming from a map. It would be good to stable sort this to prevent any kind of diffs in the future due to ordering.

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.

3 participants