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.

Sign up for GitHub

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

Jump to bottom

Add support for google_compute_node_group to TGC #10635

Merged
merged 12 commits into from
May 29, 2024
Merged

Add support for google_compute_node_group to TGC #10635

merged 12 commits into from
May 29, 2024

Conversation

delimaneto
Copy link
Contributor

@delimaneto delimaneto commented May 8, 2024 •
edited

adding support for compute.googleapis.com/NodeGroup

@github-actions GitHub Actions
Copy link

github-actions bot commented May 8, 2024

Hello! I am a robot. Tests will require approval from a repository maintainer to run.

@melinath, 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.

@github-actions github-actions bot requested a review from melinath May 8, 2024 19:42
@modular-magician modular-magician added the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label May 8, 2024
melinath
melinath requested changes May 8, 2024
View reviewed changes
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.

Since this is a TGC-only change it should use release note type none and the content of the release note should be empty. Release notes are only used for provider changes.

@modular-magician modular-magician removed the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label May 8, 2024
@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.

terraform-google-conversion: Diff ( 4 files changed, 72 insertions(+))

@delimaneto delimaneto changed the title Add Support for resource Node Group Add Suport for google_compute_node_group to TGC May 9, 2024
@delimaneto delimaneto requested a review from melinath May 9, 2024 13:30
@modular-magician modular-magician added the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label May 9, 2024
@sushant00
Copy link

@melinath I think we would need to merge #10602 first for NodeTemplate before we can merge this PR for NodeGroup.

Can you please confirm?

melinath
melinath requested changes May 9, 2024
View reviewed changes
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.

TGC converters shouldn't technically need to depend on each other (in most cases) but if there's a separate PR for node template then we should remove that from here for sure, and we could merge them in order if you prefer.

@delimaneto
Copy link
Contributor Author

TGC converters shouldn't technically need to depend on each other (in most cases) but if there's a separate PR for node template then we should remove that from here for sure, and we could merge them in order if you prefer.

I just did let NodeTemplate here because is a resource dependency for NodeGroup resource. I think we should merge the other PR about NodeTemplate and after merge here. Make sense?

@delimaneto delimaneto requested a review from melinath May 10, 2024 14:14
melinath
melinath reviewed May 10, 2024
View reviewed changes
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.

sounds good, removing from my queue for now - let me know when node template is done & I'll review this at that point.

@delimaneto delimaneto requested a review from melinath May 10, 2024 17:38
@delimaneto
Copy link
Contributor Author

node template is done, thanks.

melinath
melinath requested changes May 13, 2024
View reviewed changes
mmv1/templates/tgc/resource_converters.go.erb Outdated Show resolved Hide resolved
mmv1/templates/tgc/resource_converters.go.erb Outdated Show resolved Hide resolved
@delimaneto @melinath
Update mmv1/templates/tgc/resource_converters.go.erb
47ee5fe 
Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>
@github-actions github-actions bot requested a review from melinath May 14, 2024 11:15
@melinath melinath changed the title Add Suport for google_compute_node_group to TGC Add Support for google_compute_node_group to TGC May 15, 2024
@melinath melinath changed the title Add Support for google_compute_node_group to TGC Add support for google_compute_node_group to TGC May 15, 2024
melinath
melinath requested changes May 15, 2024
View reviewed changes
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.

Please undo the whitespace changes in resource_converters (or do them in a separate PR) - that's going to make this keep running into merge conflicts.

mmv1/templates/tgc/resource_converters.go.erb Outdated
@@ -92,7 +92,9 @@ func ResourceConverters() map[string][]cai.ResourceConverter {
"google_storage_bucket_iam_policy": {resourceConverterStorageBucketIamPolicy()},
"google_storage_bucket_iam_binding": {resourceConverterStorageBucketIamBinding()},
"google_storage_bucket_iam_member": {resourceConverterStorageBucketIamMember()},
"google_cloud_tasks_queue": {cloudtasks.ResourceConverterCloudTasksQueue()},
"google_compute_node_group": {compute.ResourceConverterComputeNodeGroup()},
"google_compute_node_template": {compute.ResourceConverterComputeNodeTemplate()},
Copy link
Member

Choose a reason for hiding this comment

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

This is still adding node_template. Please remove this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if I remove google_compute_node_template, it will not test node template resource anymore. I can't remove it.

Copy link
Member

Choose a reason for hiding this comment

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

google_compute_node_template support was added in #10602, so it doesn't also need to be added here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, done. Thanks

@melinath
Copy link
Member

Also please resolve the merge conflicts

@github-actions github-actions bot requested a review from melinath May 16, 2024 11:31
melinath
melinath requested changes May 16, 2024
View reviewed changes
mmv1/templates/tgc/resource_converters.go.erb Outdated
@@ -92,7 +92,9 @@ func ResourceConverters() map[string][]cai.ResourceConverter {
"google_storage_bucket_iam_policy": {resourceConverterStorageBucketIamPolicy()},
"google_storage_bucket_iam_binding": {resourceConverterStorageBucketIamBinding()},
"google_storage_bucket_iam_member": {resourceConverterStorageBucketIamMember()},
"google_cloud_tasks_queue": {cloudtasks.ResourceConverterCloudTasksQueue()},
"google_compute_node_group": {compute.ResourceConverterComputeNodeGroup()},
"google_compute_node_template": {compute.ResourceConverterComputeNodeTemplate()},
Copy link
Member

Choose a reason for hiding this comment

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

google_compute_node_template support was added in #10602, so it doesn't also need to be added here.

melinath
melinath requested changes May 16, 2024
View reviewed changes
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.

why are there more whitespace changes? Also, in addition to explaining why, please remove them.

@delimaneto
Copy link
Contributor Author

why are there more whitespace changes? Also, in addition to explaining why, please remove them.

There are more whitespace changes because I'm removing those on edit file github. I'm resolving the conflicts and sometimes it comes from main branch and I need to fix all of them.

@delimaneto delimaneto requested a review from melinath May 16, 2024 19:47
melinath
melinath requested changes May 16, 2024
View reviewed changes
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.

Please undo the whitespace changes

@delimaneto delimaneto requested a review from melinath May 16, 2024 21:59
@delimaneto
Copy link
Contributor Author

Please undo the whitespace changes

done, thanks

@modular-magician modular-magician removed the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label May 16, 2024
@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.

terraform-google-conversion: Diff ( 4 files changed, 71 insertions(+))

@github-actions GitHub Actions
Copy link

This PR has been waiting for review for 2 days. Please take a look! Use the label disable-review-reminders to disable these notifications.

melinath
melinath requested changes May 20, 2024
View reviewed changes
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.

Taking a look at this, it's only exercising a few of the fields on node_group. Could you add a more complete set of fields? As many as possible, ideally. https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/compute_node_group lists the available fields.

@delimaneto
Copy link
Contributor Author

Taking a look at this, it's only exercising a few of the fields on node_group. Could you add a more complete set of fields? As many as possible, ideally. https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/compute_node_group lists the available fields.

@melinath we are using the most simple group of fields that resource can once those are just to test the specific resource. As much more simpler, it'll be better as internal team said.

@delimaneto delimaneto requested a review from melinath May 21, 2024 12:32
melinath
melinath requested changes May 21, 2024
View reviewed changes
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 only other resource that could be referenced from a node group is a project, which doesn't seem like an excessive thing to add to this configuration. Please add all fields to the test. Thanks!

@github-actions github-actions bot requested a review from melinath May 22, 2024 13:27
@modular-magician modular-magician added the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label May 22, 2024
@delimaneto
Copy link
Contributor Author

the only other resource that could be referenced from a node group is a project, which doesn't seem like an excessive thing to add to this configuration. Please add all fields to the test. Thanks!

added more fields, thanks.

@delimaneto
Copy link
Contributor Author

@melinath could you please review again?

@github-actions GitHub Actions
Copy link

This PR has been waiting for review for 2 weekdays. Please take a look! Use the label disable-review-reminders to disable these notifications.

melinath
melinath approved these changes May 24, 2024
View reviewed changes
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.

LGTM assuming tests pass

@modular-magician modular-magician removed the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label May 24, 2024
@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.

terraform-google-conversion: Diff ( 4 files changed, 90 insertions(+))

@delimaneto
Copy link
Contributor Author

@melinath could you please make the merge?

@melinath melinath merged commit b8b4ffe into GoogleCloudPlatform:main May 29, 2024
11 checks passed
@melinath
Copy link
Member

yep, thanks for the ping!

@modular-magician modular-magician mentioned this pull request May 29, 2024
Merged
Cheriit pushed a commit to Cheriit/magic-modules that referenced this pull request Jun 4, 2024
@delimaneto @melinath @Cheriit
Add support for google_compute_node_group to TGC (GoogleCloudPlatfo…
b4304a9 
…rm#10635)

Co-authored-by: Stephen Lewis (Burrows) <stephen.r.burrows@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@melinath melinath melinath approved these changes

@sushant00 sushant00 sushant00 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@delimaneto @modular-magician @sushant00 @melinath