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

fix: Convert SingleNestedBlocks to ListNestedBlocks and fix arg required states for aws_paymentcryptography_key #41379

Conversation

acwwat
Copy link
Contributor

@acwwat acwwat commented Feb 13, 2025

Description

This PR is to converted all SingleNestedBlocks to ListNestedBlocks and properly set the required states for affected arguments in the aws_paymentcryptography_key resource.

Relations

Relates #35813

References

n/a

Output from Acceptance Testing

$ make testacc TESTS=TestAccPaymentCryptographyKey_ PKG=paymentcryptography
make: Verifying source code with gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.23.5 test ./internal/service/paymentcryptography/... -v -count 1 -parallel 20 -run='TestAccPaymentCryptographyKey_'  -timeout 360m -vet=off
2025/02/12 23:01:08 Initializing Terraform AWS Provider...
=== RUN   TestAccPaymentCryptographyKey_basic
=== PAUSE TestAccPaymentCryptographyKey_basic
=== RUN   TestAccPaymentCryptographyKey_tags
=== PAUSE TestAccPaymentCryptographyKey_tags
=== RUN   TestAccPaymentCryptographyKey_update
=== PAUSE TestAccPaymentCryptographyKey_update
=== RUN   TestAccPaymentCryptographyKey_disappears
=== PAUSE TestAccPaymentCryptographyKey_disappears
=== CONT  TestAccPaymentCryptographyKey_basic
=== CONT  TestAccPaymentCryptographyKey_update
=== CONT  TestAccPaymentCryptographyKey_disappears
=== CONT  TestAccPaymentCryptographyKey_tags
--- PASS: TestAccPaymentCryptographyKey_disappears (16.59s)
--- PASS: TestAccPaymentCryptographyKey_basic (19.04s)
--- PASS: TestAccPaymentCryptographyKey_tags (29.45s)
--- PASS: TestAccPaymentCryptographyKey_update (38.66s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/paymentcryptography        38.883s

$

@acwwat acwwat requested a review from a team as a code owner February 13, 2025 04:07
Copy link

Community Note

Voting for Prioritization

  • Please vote on this pull request by adding a 👍 reaction to the original post to help the community and maintainers prioritize this pull request.
  • Please see our prioritization guide for information on how we prioritize.
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request.

For Submitters

  • Review the contribution guide relating to the type of change you are making to ensure all of the necessary steps have been taken.
  • For new resources and data sources, use skaff to generate scaffolding with comments detailing common expectations.
  • Whether or not the branch has been rebased will not impact prioritization, but doing so is always a welcome surprise.

@github-actions github-actions bot added tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. service/paymentcryptography Issues and PRs that pertain to the paymentcryptography service. needs-triage Waiting for first response or review from a maintainer. external-maintainer Contribution from a trusted external contributor. labels Feb 13, 2025
@acwwat acwwat force-pushed the b-aws_paymentcryptography_key-convert_single_nested_blocks_to_list_nested_blocks branch from e9425c7 to dbc4fc6 Compare February 13, 2025 04:09
@justinretzolk justinretzolk added bug Addresses a defect in current functionality. and removed needs-triage Waiting for first response or review from a maintainer. labels Feb 13, 2025
Copy link
Member

@jar-b jar-b left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

% make testacc PKG=paymentcryptography TESTS=TestAccPaymentCryptographyKey_
make: Verifying source code with gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.23.5 test ./internal/service/paymentcryptography/... -v -count 1 -parallel 20 -run='TestAccPaymentCryptographyKey_'  -timeout 360m -vet=off
2025/02/14 11:23:46 Initializing Terraform AWS Provider...

--- PASS: TestAccPaymentCryptographyKey_disappears (13.42s)
--- PASS: TestAccPaymentCryptographyKey_basic (15.72s)
--- PASS: TestAccPaymentCryptographyKey_tags (23.99s)
--- PASS: TestAccPaymentCryptographyKey_update (32.08s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/paymentcryptography        38.771s

@@ -0,0 +1,3 @@
```release-note:bug
resource/aws_paymentcryptography_key: Set `key_attributes` and `key_attributes.*.key_modes_of_use` arguments to required
Copy link
Member

Choose a reason for hiding this comment

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

@jar-b
Copy link
Member

jar-b commented Feb 14, 2025

Thanks for the clean up, @acwwat! 👍

@jar-b jar-b merged commit 44e7b1b into hashicorp:main Feb 14, 2025
43 checks passed
@github-actions github-actions bot added this to the v5.88.0 milestone Feb 14, 2025
@jar-b
Copy link
Member

jar-b commented Feb 14, 2025

After additional review from the team, this will need to be reverted as changing from SingleNestedBlock to ListNestedBlock causes changes to the underlying schema which must be accounted for via a state upgrader.

For example, and upgrade test fails as follows.

func TestAccPaymentCryptographyKey_v5_87_0_upgrade(t *testing.T) {
	ctx := acctest.Context(t)
	if testing.Short() {
		t.Skip("skipping long-running test in short mode")
	}

	var key paymentcryptography.GetKeyOutput
	rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
	resourceName := "aws_paymentcryptography_key.test"

	resource.ParallelTest(t, resource.TestCase{
		PreCheck: func() {
			acctest.PreCheck(ctx, t)
			testAccPreCheck(ctx, t)
		},
		ErrorCheck:   acctest.ErrorCheck(t, names.PaymentCryptographyServiceID),
		CheckDestroy: testAccCheckKeyDestroy(ctx),
		Steps: []resource.TestStep{
			{
				ExternalProviders: map[string]resource.ExternalProvider{
					"aws": {
						Source:            "hashicorp/aws",
						VersionConstraint: "5.87.0",
					},
				},
				Config: testAccKeyConfig_basic(rName),
				Check: resource.ComposeTestCheckFunc(
					testAccCheckKeyExists(ctx, resourceName, &key),
				),
			},
			{
				ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories,
				Config:                   testAccKeyConfig_basic(rName),
				Check: resource.ComposeTestCheckFunc(
					testAccCheckKeyExists(ctx, resourceName, &key),
					resource.TestCheckResourceAttr(resourceName, names.AttrEnabled, acctest.CtTrue),
					resource.TestCheckResourceAttr(resourceName, "exportable", acctest.CtTrue),
					resource.TestCheckResourceAttr(resourceName, "key_attributes.#", "1"),
					resource.TestCheckResourceAttr(resourceName, "key_attributes.0.key_algorithm", "TDES_3KEY"),
					resource.TestCheckResourceAttr(resourceName, "key_attributes.0.key_class", "SYMMETRIC_KEY"),
					resource.TestCheckResourceAttr(resourceName, "key_attributes.0.key_usage", "TR31_P0_PIN_ENCRYPTION_KEY"),
					resource.TestCheckResourceAttr(resourceName, "key_attributes.0.key_modes_of_use.#", "1"),
					resource.TestCheckResourceAttr(resourceName, "key_attributes.0.key_modes_of_use.0.decrypt", acctest.CtTrue),
					resource.TestCheckResourceAttr(resourceName, "key_attributes.0.key_modes_of_use.0.encrypt", acctest.CtTrue),
					resource.TestCheckResourceAttr(resourceName, "key_attributes.0.key_modes_of_use.0.wrap", acctest.CtTrue),
					resource.TestCheckResourceAttr(resourceName, "key_attributes.0.key_modes_of_use.0.unwrap", acctest.CtTrue),
					acctest.MatchResourceAttrRegionalARN(ctx, resourceName, names.AttrARN, "payment-cryptography", regexache.MustCompile(`key/.+`)),
				),
			},
		},
	})
}
% make testacc PKG=paymentcryptography TESTS=TestAccPaymentCryptographyKey_v5_87_0_upgrade
make: Verifying source code with gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.23.5 test ./internal/service/paymentcryptography/... -v -count 1 -parallel 20 -run='TestAccPaymentCryptographyKey_v5_87_0_upgrade'  -timeout 360m -vet=off
2025/02/14 13:49:10 Initializing Terraform AWS Provider...
=== RUN   TestAccPaymentCryptographyKey_v5_87_0_upgrade
=== PAUSE TestAccPaymentCryptographyKey_v5_87_0_upgrade
=== CONT  TestAccPaymentCryptographyKey_v5_87_0_upgrade
    key_test.go:37: Step 2/2 error: Error running pre-apply plan: exit status 1

        Error: Unable to Read Previously Saved State for UpgradeResourceState

          with aws_paymentcryptography_key.test,
          on terraform_plugin_test.tf line 12, in resource "aws_paymentcryptography_key" "test":
          12: resource "aws_paymentcryptography_key" "test" {

        There was an error reading the saved resource state using the current
        resource schema.

        If this resource state was last refreshed with Terraform CLI 0.11 and
        earlier, it must be refreshed or applied with an older provider version
        first. If you manually modified the resource state, you will need to manually
        modify it to match the current resource schema. Otherwise, please report this
        to the provider developer:

        AttributeName("key_attributes"): invalid JSON, expected "[", got "{"
    panic.go:629: Error retrieving state, there may be dangling resources: exit status 1
        Failed to marshal state to json: missing expected [
--- FAIL: TestAccPaymentCryptographyKey_v5_87_0_upgrade (29.76s)
FAIL
FAIL    github.com/hashicorp/terraform-provider-aws/internal/service/paymentcryptography        36.457s

I'll open a PR shortly to revert this. We can open a new PR with this same change that includes a state upgrader and slot if for inclusion as part of the v6.0.0 major release.

jar-b added a commit that referenced this pull request Feb 14, 2025
…rg required states for aws_paymentcryptography_key (#41379)"

This reverts commit 44e7b1b.

After additional review from the team, this will need to be reverted as changing from `SingleNestedBlock` to `ListNestedBlock` causes changes to the underlying schema which must be accounted for via a state upgrader. See [this comment](#41379 (comment)) for additional details.
jar-b added a commit that referenced this pull request Feb 14, 2025
…rg required states for aws_paymentcryptography_key (#41379)" (#41402)

This reverts commit 44e7b1b.

After additional review from the team, this will need to be reverted as changing from `SingleNestedBlock` to `ListNestedBlock` causes changes to the underlying schema which must be accounted for via a state upgrader. See [this comment](#41379 (comment)) for additional details.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Addresses a defect in current functionality. external-maintainer Contribution from a trusted external contributor. service/paymentcryptography Issues and PRs that pertain to the paymentcryptography service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants