Add for storage_pools flag on cluster/nodepool create, and nodepool update #11391
Conversation
|
Hello! I am a robot. Tests will require approval from a repository maintainer to run. @shuyama1, 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
added
the
awaiting-approval
amacaskill
force-pushed
the
main
branch
2 times, most recently
from
db3a745 to
94f7f22
Compare
modular-magician
added
service/container
and removed
awaiting-approval
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Errors
|
Tests analyticsTotal tests: 0 Click here to see the affected service packages
Tests were added that are skipped in VCR:
View the build log |
There was a problem hiding this comment.
Looks like the provider failed to compile. Probably due to errors stated in the unit tests https://github.com/GoogleCloudPlatform/magic-modules/actions/runs/10315416311/job/28555548530?pr=11391. Please let me know if you need help debugging. Thanks
|
@shuyama1 Looks like the most recent release is 0v.191.0 and it doesn't have storagePools field yet in NodeConfig. https://pkg.go.dev/google.golang.org/[email protected]/container/v1#NodeConfig. I'm hoping it will make it into the google.golang.org/api release for next week, or the following week. |
There was a problem hiding this comment.
@shuyama1 Looks like the most recent release is 0v.191.0 and it doesn't have storagePools field yet in NodeConfig. pkg.go.dev/google.golang.org/[email protected]/container/v1#NodeConfig. I'm hoping it will make it into the google.golang.org/api release for next week, or the following week.
Oh, that makes sense. Thanks for checking! After the storagePools field is released, you may want to follow the instruction to make a PR to update the dependencies in providers first and get that PR merged and then continue with the work on this feature in this PR. Please let me know if you have any questions. Thanks!
modular-magician
added
the
awaiting-approval
I added PR to update the generated client: #11520. After this is merged, I will rebase this PR on main |
There was a problem hiding this comment.
@shuyama1 Looks like the most recent release is 0v.191.0 and it doesn't have storagePools field yet in NodeConfig. pkg.go.dev/google.golang.org/[email protected]/container/v1#NodeConfig. I'm hoping it will make it into the google.golang.org/api release for next week, or the following week.
Oh, that makes sense. Thanks for checking! After the storagePools field is released, you may want to follow the instruction to make a PR to update the dependencies in providers first and get that PR merged and then continue with the work on this feature in this PR. Please let me know if you have any questions. Thanks!
I added PR to update the generated client: #11520. After this is merged, I will rebase this PR on main
Thank you! I'll trigger the test for this PR after #11520 is merged
modular-magician
removed
the
awaiting-approval
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 3907 Click here to see the affected service packages
Action takenFound 639 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
I rebased the PR onto #11520, but both PRs are failing " |
The failure is not related to your change. It's already failed at head. Please ignore them. Sorry for the noise. |
|
VCR tests should be fixed. Triggering a rerun now. /gcbrun |
modular-magician
added
awaiting-approval
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 208 Click here to see the affected service packages
Action takenFound 4 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
|
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 208 Click here to see the affected service packages
Action takenFound 4 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
|
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 208 Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
mmv1/third_party/terraform/services/container/resource_container_cluster_test.go.erb
Outdated
Show resolved
Hide resolved
| node_config { | ||
| machine_type = "c3-standard-4" | ||
| image_type = "COS_CONTAINERD" | ||
| storage_pools = ["%[5]s"] |
There was a problem hiding this comment.
I assume that this field accepts long form name? Let's keep the extractSPName for now as it shouldn't block the merge. I'll modify the bootstrap function to return the long form name and update the tests later.
There was a problem hiding this comment.
It doesn't accept URL name, only the long resource name. "projects/project/zones/zone/storagePools/SPName
There was a problem hiding this comment.
Got it. Thanks for confirming! I'll update the bootstrap function later. I initially intended for it to return the long form name but noticed it currently returns the URL instead.
There was a problem hiding this comment.
I think this is now causing problems since it seems to have moved from v1 to beta? Maybe there's a reason it wasn't done this way to start with, but threw up #11832 for consideration
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 208 Click here to see the affected service packages
View the build log |
Tests such as `TestAccContainerNodePool_withDiskMachineAndStoragePoolUpdate` seem to be failing, possibly due to being promoted from beta to v1 Rather than update the regex in extractSP, use `tpgresource.GetRelativePath()` and return that from the bootstrap method, as mentioned here: GoogleCloudPlatform#11391 (comment) See GoogleCloudPlatform#11391 and GoogleCloudPlatform#11598
Add terraform support for storage_pools on cluster/nodepool create, and nodepool update.
Release Note Template for Downstream PRs (will be copied)