-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 support for aliases like composer-2 in Composer image version field #5787
Conversation
Hello! I am a robot who works on Magic Modules PRs. I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review. Thanks for your contribution! A human will be with you soon. @c2thorn, please review this PR or find an appropriate assignee. |
I prepared this draft PR but we shouldn't merge it now because Cloud Composer doesn't support the new aliases yet. |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 4 files changed, 76 insertions(+), 40 deletions(-)) |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDataSourceSqlDatabaseInstance_basic|TestAccBigqueryReservationAssignment_BasicHandWritten|TestAccCGCSnippet_sqlSqlserverInstanceAuthorizedNetworkExample|TestAccCloudBuildTrigger_cloudbuildTriggerFilenameExample|TestAccCloudBuildTrigger_cloudbuildTriggerBuildExample|TestAccCloudBuildTrigger_basic|TestAccCloudBuildTrigger_available_secrets_config|TestAccCloudBuildTrigger_cloudbuildTriggerServiceAccountExample|TestAccCloudBuildTrigger_disable|TestAccCloudBuildTrigger_fullStep|TestAccContainerCluster_withAuthenticatorGroupsConfig You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=264759 |
Tests failed during RECORDING mode: TestAccContainerCluster_withAuthenticatorGroupsConfig Please fix these to complete your PR |
The new aliases are now available in Cloud Composer so the change can be merged. Please review, thanks! |
/gcbrun |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 4 files changed, 76 insertions(+), 40 deletions(-)) |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDatasourceGoogleServiceNetworkingPeeredDnsDomain_basic|TestAccApikeysKey_AndroidKey|TestAccApikeysKey_BasicKey|TestAccApikeysKey_IosKey|TestAccApikeysKey_MinimalKey|TestAccApikeysKey_ServerKey|TestAccArtifactRegistryRepository_create_mvn_snapshot|TestAccArtifactRegistryRepository_create_mvn_release|TestAccCGCSnippet_sqlMysqlInstanceBackupExample|TestAccCGCSnippet_sqlPostgresInstanceBackupExample|TestAccCGCSnippet_sqlSqlserverInstanceBackupExample|TestAccCGCSnippet_sqlMysqlInstanceAuthorizedNetworkExample|TestAccCGCSnippet_sqlSqlserverInstanceBackupLocationExample|TestAccCloudBuildTrigger_cloudbuildTriggerPubsubConfigExample|TestAccCloudBuildTrigger_cloudbuildTriggerWebhookConfigExample|TestAccCloudBuildTrigger_cloudbuildTriggerManualExample|TestAccCloudFunctionsFunction_secretEnvVar|TestAccContainerAwsCluster_BasicHandWritten|TestAccContainerCluster_withAuthenticatorGroupsConfig|TestAccContainerNodePool_gvnic|TestAccDataprocCluster_nonPreemptibleSecondary|TestAccDataprocCluster_updatable|TestAccDataprocCluster_withConfigOverrides|TestAccNetworkServicesEdgeCacheOrigin_networkServicesEdgeCacheOriginAdvancedExample|TestAccNetworkServicesEdgeCacheOrigin_updateAndImport|TestAccNetworkServicesEdgeCacheService_networkServicesEdgeCacheServiceAdvancedExample|TestAccNetworkServicesEdgeCacheService_updateAndImport You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=268594 |
This covers the new aliases for Composer major version ('composer-1', 'composer-2'), improved DiffSuppressFunc implementation for image version field, and updates to the related documentation.
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 4 files changed, 76 insertions(+), 40 deletions(-)) |
Today I synced my branch with upstream but |
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.
When a user specifies an alias, will the API return the non-aliased version? I assume yes because of the diff suppress, but just wanted to confirm.
Can we add an alias to one of the tests other than the DiffSuppress test? I want to ensure it is tested in something that actually deploys Composer resources. Nevermind on this, the import verify step would fail because it doesn't apply diff suppresses.
/gcbrun |
Yes, Composer API always returns a fully resolved image version for a created environment. I thought about modifying one of the tests that create a Composer 2 environment to use the new alias. But we still need to set the Airflow major and minor versions, and these may change overtime for the most recent Composer 2 version. |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 4 files changed, 76 insertions(+), 40 deletions(-)) |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccCGCSnippet_sqlSqlserverInstanceBackupLocationExample|TestAccContainerCluster_withAuthenticatorGroupsConfig|TestAccLoggingLogView_BasicHandWritten|TestAccNetworkServicesEdgeCacheService_networkServicesEdgeCacheServiceAdvancedExample|TestAccNetworkServicesEdgeCacheService_updateAndImport|TestAccOSConfigPatchDeployment_osConfigPatchDeploymentDailyMidnightExample You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=269572 |
None of the failing tests are related to composer, merging. |
Tests failed during RECORDING mode: TestAccContainerCluster_withAuthenticatorGroupsConfig Please fix these to complete your PR |
This covers the new aliases for Composer major version (
composer-1
,composer-2
), improvedDiffSuppressFunc
implementation for image version field, and updates to the related documentation.Fixes hashicorp/terraform-provider-google#11217
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)