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

Upgrade terraform provider google beta to v6.0.1 major #2355

Merged

Conversation

guineveresaenger
Copy link
Contributor

@guineveresaenger guineveresaenger commented Aug 28, 2024

Upgrades upstream provider to v6.0.1.

Resolves #2348.
Resolves #2357
- Adds a patch for renaming new default label to goog-pului-provisioned
- Renames config field to addPulumiAttributionLabel
- Adjusts labels tests to expect new label
- Adjusts bucket label patch to avoid writing this label to the Bucket resource itself.

Introduces additional breaking change via cloudrunv2.Service ports field, which is now a MaxItemsOne upstream, which turns it into an object on our end, rather than an array entry. Look for a state transformer on the resource for upgrades.

Removes patches:

@guineveresaenger guineveresaenger added the needs-release/major When a PR with this label merges, it initiates a release of vX+1.0.0 label Aug 28, 2024
Copy link

github-actions bot commented Aug 28, 2024

Does the PR have any schema changes?

Found 52 breaking changes:

Resources

  • "gcp:alloydb/cluster:Cluster":
    • 🟡 inputs: "network" missing
    • 🟡 properties: "network" missing output "network"
  • "gcp:applicationintegration/client:Client":
    • inputs:
      • 🟡 "createSampleWorkflows" missing
      • 🟡 "provisionGmek" missing
    • properties:
      • 🟡 "createSampleWorkflows" missing output "createSampleWorkflows"
      • 🟡 "provisionGmek" missing output "provisionGmek"
  • "gcp:bigquery/reservation:Reservation":
    • 🟡 inputs: "multiRegionAuxiliary" missing
    • 🟡 properties: "multiRegionAuxiliary" missing output "multiRegionAuxiliary"
  • "gcp:bigquery/table:Table":
    • 🟡 inputs: "allowResourceTagsOnDeletion" missing
    • 🟡 properties: "allowResourceTagsOnDeletion" missing output "allowResourceTagsOnDeletion"
  • 🟡 "gcp:compute/managedSslCertificate:ManagedSslCertificate": inputs: "certificateId" missing
  • 🟡 "gcp:compute/mangedSslCertificate:MangedSslCertificate": inputs: "certificateId" missing
  • 🔴 "gcp:datastore/dataStoreIndex:DataStoreIndex" missing
  • "gcp:gkeonprem/vMwareCluster:VMwareCluster":
    • 🟡 inputs: "vcenters" missing
    • 🟡 properties: "vcenters" missing output "vcenters"
  • 🔴 "gcp:identityplatform/projectDefaultConfig:ProjectDefaultConfig" missing
  • "gcp:organizations/project:Project":
    • 🟡 inputs: "skipDelete" missing
    • 🟡 properties: "skipDelete" missing output "skipDelete"
  • 🟢 "gcp:pubsub/topic:Topic": required: "schemaSettings" property is no longer Required

Types

  • 🟡 "gcp:cloudrunv2/ServiceTemplateContainer:ServiceTemplateContainer": properties: "ports" type changed from "array" to "#/types/gcp:cloudrunv2/ServiceTemplateContainerPorts:ServiceTemplateContainerPorts":
    • 🟡 items had &{Type: Ref:#/types/gcp:cloudrunv2/ServiceTemplateContainerPort:ServiceTemplateContainerPort AdditionalProperties: Items: OneOf:[] Discriminator: Plain:false} but now has no type
  • 🔴 "gcp:cloudrunv2/ServiceTemplateContainerPort:ServiceTemplateContainerPort" missing
  • "gcp:compute/BackendServiceIap:BackendServiceIap": required:
    • 🟢 "enabled" property has changed to Required
    • 🟢 "oauth2ClientId" property is no longer Required
    • 🟢 "oauth2ClientSecret" property is no longer Required
  • "gcp:compute/RegionBackendServiceIap:RegionBackendServiceIap": required:
    • 🟢 "enabled" property has changed to Required
    • 🟢 "oauth2ClientId" property is no longer Required
    • 🟢 "oauth2ClientSecret" property is no longer Required
  • 🟢 "gcp:compute/SubnetworkSecondaryIpRange:SubnetworkSecondaryIpRange": required: "ipCidrRange" property is no longer Required
  • 🟢 "gcp:compute/getBackendServiceIap:getBackendServiceIap": required: "enabled" property has changed to Required
  • 🟢 "gcp:compute/getForwardingRulesRule:getForwardingRulesRule": required: "forwardingRuleId" property has changed to Required
  • 🟡 "gcp:container/ClusterMonitoringConfig:ClusterMonitoringConfig": properties: "advancedDatapathObservabilityConfigs" missing
  • "gcp:container/ClusterMonitoringConfigAdvancedDatapathObservabilityConfig:ClusterMonitoringConfigAdvancedDatapathObservabilityConfig":
    • 🟡 properties: "relayMode" missing
    • 🟢 required: "enableRelay" property has changed to Required
  • "gcp:container/getClusterMonitoringConfigAdvancedDatapathObservabilityConfig:getClusterMonitoringConfigAdvancedDatapathObservabilityConfig":
    • 🟡 properties: "relayMode" missing
    • 🟢 required: "relayMode" property is no longer Required
  • 🔴 "gcp:datastore/DataStoreIndexProperty:DataStoreIndexProperty" missing
  • 🟡 "gcp:gkeonprem/VMwareNodePoolConfig:VMwareNodePoolConfig": properties: "vsphereConfigs" missing
  • 🔴 "gcp:identityplatform/ProjectDefaultConfigSignIn:ProjectDefaultConfigSignIn" missing
  • 🔴 "gcp:identityplatform/ProjectDefaultConfigSignInAnonymous:ProjectDefaultConfigSignInAnonymous" missing
  • 🔴 "gcp:identityplatform/ProjectDefaultConfigSignInEmail:ProjectDefaultConfigSignInEmail" missing
  • 🔴 "gcp:identityplatform/ProjectDefaultConfigSignInHashConfig:ProjectDefaultConfigSignInHashConfig" missing
  • 🔴 "gcp:identityplatform/ProjectDefaultConfigSignInPhoneNumber:ProjectDefaultConfigSignInPhoneNumber" missing
  • 🟡 "gcp:sql/DatabaseInstanceSettingsIpConfiguration:DatabaseInstanceSettingsIpConfiguration": properties: "requireSsl" missing
  • "gcp:sql/getDatabaseInstanceSettingIpConfiguration:getDatabaseInstanceSettingIpConfiguration":
    • 🟡 properties: "requireSsl" missing
    • 🟢 required: "requireSsl" property is no longer Required
  • "gcp:sql/getDatabaseInstancesInstanceSettingIpConfiguration:getDatabaseInstancesInstanceSettingIpConfiguration":
    • 🟡 properties: "requireSsl" missing
    • 🟢 required: "requireSsl" property is no longer Required
  • 🟡 "gcp:storage/BucketLifecycleRuleCondition:BucketLifecycleRuleCondition": properties: "noAge" missing
  • "gcp:storage/getBucketLifecycleRuleCondition:getBucketLifecycleRuleCondition":
    • 🟡 properties: "noAge" missing
    • required:
      • 🟢 "noAge" property is no longer Required
      • 🟢 "sendAgeIfZero" property has changed to Required

New resources:

  • kms/ekmConnectionIamBinding.EkmConnectionIamBinding
  • kms/ekmConnectionIamMember.EkmConnectionIamMember
  • kms/ekmConnectionIamPolicy.EkmConnectionIamPolicy
  • securitycenter/v2OrganizationSccBigQueryExports.V2OrganizationSccBigQueryExports

New functions:

  • kms/getEkmConnectionIamPolicy.getEkmConnectionIamPolicy

Maintainer note: consult the runbook for dealing with any breaking changes.

@VenelinMartinov
Copy link
Contributor

VenelinMartinov commented Aug 29, 2024

For the 0002 patch: #2319 (comment)

I'm concerned that it's still an issue since the underlying bridge problem is not fixed - could we add a regression test with a partial state .get?

@guineveresaenger guineveresaenger force-pushed the upgrade-terraform-provider-google-beta-to-v6.0.1-major branch from 8f3714f to 92b0582 Compare August 29, 2024 19:55
provider/resources.go Outdated Show resolved Hide resolved
@guineveresaenger guineveresaenger force-pushed the upgrade-terraform-provider-google-beta-to-v6.0.1-major branch 4 times, most recently from c6939a2 to 3382ff7 Compare September 10, 2024 00:33
@guineveresaenger guineveresaenger marked this pull request as ready for review September 10, 2024 16:54
Copy link
Member

@iwahbe iwahbe left a comment

Choose a reason for hiding this comment

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

Looks good to me. Only 2 nits:

  • The comment about TransormFromState.
  • The TODO still remaining.

Otherwise I think this is good to merge.

provider/provider_yaml_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@VenelinMartinov VenelinMartinov left a comment

Choose a reason for hiding this comment

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

Great! Can we add an upgrade test for the new state migration?

Also the change in TestOrganizationsProjectAutoNaming looks like it will leak resources.

Can we also separate the patch removals in a new PR? They don't look like they interact with the major version upgrade.

{"cloudrunv2-service-7"},
// TODO: This upgrade will no longer be possible, as it touches multiple new changes. Must be re-recorded.
// Skipping for now.
//{"cloudrunv2-service-7"},
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we re-recording it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll file a follow-up issue and link it here. 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the only test which has a non-null value for the migrated parameter, we should fix it or write a new one to make sure that the migration works for users.

@guineveresaenger
Copy link
Contributor Author

@VenelinMartinov - I believe the state transformation is already covered by the cloudrunv2-service-4 and cloudrunv2-service-5 upgrade tests - their failure is what made us aware of this breaking change. Their passing is proof the state migration works.

@VenelinMartinov
Copy link
Contributor

VenelinMartinov commented Sep 11, 2024

Hm, neither cloudrunv2-service-4 or cloudrunv2-service-5 actually use the parameter - we should either fix service-7 which uses the ports parameter or write a new test which shows that the migration works for users.

@guineveresaenger guineveresaenger force-pushed the upgrade-terraform-provider-google-beta-to-v6.0.1-major branch from 7f98067 to 50564c0 Compare September 11, 2024 17:35
@guineveresaenger guineveresaenger force-pushed the upgrade-terraform-provider-google-beta-to-v6.0.1-major branch from 50564c0 to f83416f Compare September 11, 2024 18:00
@guineveresaenger
Copy link
Contributor Author

@VenelinMartinov - the state migration is specifically for a default port parameter, which is not visible in the code.

We cannot "fix" service-7, because this is a breaking change due to MaxItemsOne. When ports is explicitly specified in pulumi code, users must re-write the field to be an Object, not an Array item. This is noted here.

@VenelinMartinov
Copy link
Contributor

Discussed offline and found @corymhall's code which tests exactly the case we need here: https://github.com/pulumi/pulumi-aws/blob/de5418dbd55dcc23ccc45e756b258bf4e12165da/provider/provider_yaml_test.go#L322

Upgrade test where the new program needs to change compared to the old one.

@guineveresaenger guineveresaenger force-pushed the upgrade-terraform-provider-google-beta-to-v6.0.1-major branch 2 times, most recently from 2ae64aa to 242aec1 Compare September 12, 2024 15:56
Copy link
Member

@iwahbe iwahbe left a comment

Choose a reason for hiding this comment

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

LGTM

Have we verified that the pulumi/examples repo passes with the new provider?

examples/labels-combinations-go/main.go Outdated Show resolved Hide resolved
.goreleaser.prerelease.yml Outdated Show resolved Hide resolved
@guineveresaenger guineveresaenger force-pushed the upgrade-terraform-provider-google-beta-to-v6.0.1-major branch from 242aec1 to c29bcdb Compare September 13, 2024 17:46
@guineveresaenger guineveresaenger merged commit c8582e0 into master Sep 13, 2024
46 checks passed
@guineveresaenger guineveresaenger deleted the upgrade-terraform-provider-google-beta-to-v6.0.1-major branch September 13, 2024 18:53
@pulumi-bot
Copy link
Contributor

This PR has been shipped in release v8.0.0.

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.

Handle new default label Upgrade terraform-provider-google-beta to v6.0.1
4 participants