-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add GKE Multicloud #5522
Add GKE Multicloud #5522
Conversation
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 38 files changed, 8038 insertions(+), 2 deletions(-)) |
/gcbrun |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 38 files changed, 8038 insertions(+), 2 deletions(-)) |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDatasourceGoogleServiceNetworkingPeeredDnsDomain_basic|TestAccApigeeEnvironmentIamBindingGenerated|TestAccApigeeEnvironmentIamMemberGenerated|TestAccApigeeEnvironmentIamPolicyGenerated|TestAccCloudFunctionsFunction_vpcConnector|TestAccComputeRegionNetworkEndpointGroup_regionNetworkEndpointGroupAppengineExample|TestAccContainerAwsCluster_BasicCluster|TestAccContainerAwsCluster_MinimalCluster|TestAccContainerAwsNodePool_BasicNodePool|TestAccContainerAwsNodePool_MinimalNodePool|TestAccContainerAzureCluster_BasicCluster|TestAccContainerAzureCluster_MinimalCluster|TestAccContainerAzureNodePool_BasicNodePool|TestAccContainerAzureNodePool_MinimalNodePool|TestAccContainerCluster_errorAutopilotLocation|TestAccContainerNodePool_withInvalidUpgradeSettings You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=228882 |
Tests failed during RECORDING mode: TestAccContainerAzureCluster_MinimalCluster|TestAccContainerAzureCluster_BasicCluster|TestAccContainerAwsNodePool_MinimalNodePool|TestAccContainerAwsNodePool_BasicNodePool|TestAccContainerAwsCluster_BasicCluster|TestAccContainerAwsCluster_MinimalCluster|TestAccContainerAzureNodePool_MinimalNodePool|TestAccContainerAzureNodePool_BasicNodePool|TestAccContainerCluster_errorAutopilotLocation|TestAccCloudFunctionsFunction_vpcConnector|TestAccComputeRegionNetworkEndpointGroup_regionNetworkEndpointGroupAppengineExample|TestAccContainerNodePool_withInvalidUpgradeSettings Please fix these to complete your PR |
Needed to enable the service - https://ci-oss.hashicorp.engineering/buildConfiguration/GoogleCloudBeta_ProviderGoogleCloudBetaMmUpstreamVcr/228997 contains the next test run. |
b95e68a
to
2e552e9
Compare
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 45 files changed, 7984 insertions(+), 20 deletions(-)) |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 44 files changed, 7954 insertions(+), 2 deletions(-)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kicked off tests at: https://ci-oss.hashicorp.engineering/buildConfiguration/GoogleCloudBeta_ProviderGoogleCloudBetaMmUpstreamVcr/230432
Looks like TestAccContainerAwsNodePool_BasicHandWritten
failed due to invalid GKE versions
There seem to be some problems with fields not getting docs generated: https://github.com/modular-magician/terraform-provider-google-beta/compare/auto-pr-5522-old..auto-pr-5522#diff-23ae12e759b02f32bb44a3051c475caaa877980c393a288714647c43bbee06d5R134
Maybe the OpenAPI spec doesn't have descriptions for these fields? The sub-fields also have empty descriptions
I'm also noticing a pattern on name
fields having incorrect formats in their description. It includes the output format projects/{project}/locations/{location}/...
rather than the expected input format which is just the name of the resource. This is a fairly common problem across auto-generated DCL descriptions
|
||
The following arguments are supported: | ||
|
||
* `location` (Optional) - The location (region or zone) to list versions for. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is location going to be a GCP location or a AWS location?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is GCP location e.g. us-west1
, us-east4
etc.
nit: Could we remove "zone"? I don't think we support GCP zones.
Fixed those things - the docs were missing in the openapi spec, yep. |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 45 files changed, 7974 insertions(+), 20 deletions(-)) |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 44 files changed, 7964 insertions(+), 2 deletions(-)) |
Looks like This is the same field that previously didn't have descriptions, do we need a DCL update to pull the new field descriptions? |
Annotations and tags are listed as empty objects in the docs, which seems strange. Should these have values present in the samples? https://github.com/modular-magician/terraform-provider-google/compare/auto-pr-5522-old..auto-pr-5522?short_path=addc80c#diff-2a61e3e0e41cd564d2112d73c99bea55867cdabf41ad9bbb572b82047648272eR94 |
Yeah ... it's weird, it generates correctly on my machine. e: I see, there was a merge conflict. Let me resolve that.
Those are handwritten - I can remove them or add values, if you have a preference. |
… divergence in naming between this and OSPolicy.
7adc071
to
ee34e13
Compare
Okay, fixed the merge conflict, so the docs will show up now... Anything other than the docs? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing major once the docs are fixed. Either way on the tags/annotations fields. Removing them in most cases probably makes sense as they seem to be nearly identical across resources
I think bringing up the requirement on project number rather than ID with the API team would be useful, as ID is much easier for most users to work with. But that's not blocking for this
} | ||
|
||
fleet { | ||
project = "{{project_number}}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these values accept project ID as well? I believe they should based on AIP 2510, so it may be worth raising w/ the API team if they do not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these values accept project ID as well?
No, they currently don't accept project ID. For more context, AIP 2510 used to advocate always returning project number and the gkemulticloud API followed the old guidance. Accepting project ID could cause terraform to incorrectly detect a diff.
For our launch, we wanted to avoid making breaking API changes so we limited these values to only accept project numbers. This is something we're aware of and would consider supporting project ID post GA.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we talked about this extensively and they're aware it's not ideal for cleanliness of user configs, but it's the best we could do.
} | ||
|
||
azure_region = "westus2" | ||
client = "projects/{{project_number}}/locations/us-west1/azureClients/${google_container_azure_client.basic.name}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly this should accept a project ID to make user's lives easier
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 44 files changed, 7995 insertions(+), 19 deletions(-)) |
One note: The description for fleet.project indicates that the name of the project should be used, but because we specifically have added the project_number to the tests I'm guessing it only allows the # to be used? The description should probably reflect that if it is the case |
Okay, I'll run those tests again and check the docs before merging. |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 45 files changed, 7996 insertions(+), 20 deletions(-)) |
Great. All the tests pass and the docs look right. |
* Release GKE Multicloud in terraform at GA. * Updates to samples as we go GA. * Some additional complexities in moving to GA - regional endpoints and divergence in naming between this and OSPolicy. * Move to handwritten samples, add data source for versions and permissible regions. * Add handwritten docs for Azure and AWS container versions. * Get data sources under same header as resources. * Fix documentation + update node pool test to use the version everywhere. * fix lints - do error checking. * Update DCL version again.
* Release GKE Multicloud in terraform at GA. * Updates to samples as we go GA. * Some additional complexities in moving to GA - regional endpoints and divergence in naming between this and OSPolicy. * Move to handwritten samples, add data source for versions and permissible regions. * Add handwritten docs for Azure and AWS container versions. * Get data sources under same header as resources. * Fix documentation + update node pool test to use the version everywhere. * fix lints - do error checking. * Update DCL version again.
This PR adds GKE Multicloud to Terraform, via the DCL.
A few notes of complexity here:
If this PR is for Terraform, I acknowledge that I have:
make test
andmake lint
to ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)