`azurerm_container_registry_token_password` - Handle gone container registry token and mark this password as gone #27232

magodo · 2024-08-29T03:35:08Z

Community Note

Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
Please do not leave comments along the lines of "+1", "me too" or "any updates", they generate extra noise for PR followers and do not help prioritize for review

Description

This PR handles the case that the parent container registry token of this password has gone, and mark this resource as gone accordingly.

PR Checklist

I have followed the guidelines in our Contributing Documentation.
I have checked to ensure there aren't other open Pull Requests for the same update/change.
I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
For example: “resource_name_here - description of change e.g. adding property new_property_name_here”

Changes to existing Resource / Data Source

I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
I have written new tests for my resource or datasource changes & updated any relevent documentation.
I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.
(For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.

Testing

My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)

💢  TF_ACC=1 go test -v -timeout=20h -run="TestAccContainerRegistryTokenPassword_replace" ./internal/services/containers
=== RUN   TestAccContainerRegistryTokenPassword_replace
=== PAUSE TestAccContainerRegistryTokenPassword_replace
=== CONT  TestAccContainerRegistryTokenPassword_replace
--- PASS: TestAccContainerRegistryTokenPassword_replace (450.43s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/containers    450.443s

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

azurerm_container_registry_token_password - Handle gone container registry token and mark this password as gone [GH-00000]

This is a (please select all that apply):

Bug Fix
New Feature (ie adding a service, resource, or data source)
Enhancement
Breaking Change

Related Issue(s)

Fixes #27215

Note

If this PR changes meaningfully during the course of review please update the title and description as required.

internal/services/containers/container_registry_token_password_resource.go

…_resource.go Co-authored-by: stephybun <steph@hashicorp.com>

stephybun

There are a few test failures. Would you mind fixing those @magodo?

------- Stdout: -------
=== RUN   TestAccContainerRegistryTokenPassword_replace
=== PAUSE TestAccContainerRegistryTokenPassword_replace
=== CONT  TestAccContainerRegistryTokenPassword_replace
    testcase.go:173: Step 4/6 error: Pre-apply plan check(s) failed:
        'azurerm_container_registry_token_password.test' - expected action to not be Replace, path: [[container_registry_token_id]]
--- FAIL: TestAccContainerRegistryTokenPassword_replace (185.77s)
FAIL

------- Stdout: -------
=== RUN   TestAccContainerRegistryTokenPassword_update
=== PAUSE TestAccContainerRegistryTokenPassword_update
=== CONT  TestAccContainerRegistryTokenPassword_update
    testcase.go:173: Step 4/9 error: Pre-apply plan check(s) failed:
        'azurerm_container_registry_token_password.test' - expected action to not be Replace, path: [[password1 0 expiry] [password2 0 expiry]]
--- FAIL: TestAccContainerRegistryTokenPassword_update (185.95s)
FAIL

------- Stdout: -------
=== RUN   TestAccContainerRegistryTokenPassword_updateExpiryReflectNewValue
=== PAUSE TestAccContainerRegistryTokenPassword_updateExpiryReflectNewValue
=== CONT  TestAccContainerRegistryTokenPassword_updateExpiryReflectNewValue
    testcase.go:173: Step 4/6 error: Pre-apply plan check(s) failed:
        'azurerm_container_registry_token_password.test' - expected action to not be Replace, path: [[password1 0 expiry]]
--- FAIL: TestAccContainerRegistryTokenPassword_updateExpiryReflectNewValue (208.13s)
FAIL

…markasgone

magodo · 2024-11-11T03:05:35Z

@stephybun Thank you for pointing this out! I've merged with the main and updated the tests:

> TF_ACC=1 go test -v -timeout=20h -run='TestAccContainerRegistryTokenPassword_(replace|update)' ./internal/services/containers
=== RUN   TestAccContainerRegistryTokenPassword_update
=== PAUSE TestAccContainerRegistryTokenPassword_update
=== RUN   TestAccContainerRegistryTokenPassword_updateExpiryReflectNewValue
=== PAUSE TestAccContainerRegistryTokenPassword_updateExpiryReflectNewValue
=== RUN   TestAccContainerRegistryTokenPassword_replace
=== PAUSE TestAccContainerRegistryTokenPassword_replace
=== CONT  TestAccContainerRegistryTokenPassword_update
=== CONT  TestAccContainerRegistryTokenPassword_updateExpiryReflectNewValue
=== CONT  TestAccContainerRegistryTokenPassword_replace
--- PASS: TestAccContainerRegistryTokenPassword_updateExpiryReflectNewValue (272.62s)
--- PASS: TestAccContainerRegistryTokenPassword_replace (305.17s)
--- PASS: TestAccContainerRegistryTokenPassword_update (310.24s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/containers    310.269s

stephybun · 2024-11-11T08:48:06Z

internal/services/containers/container_registry_token_password_resource_test.go

@@ -56,7 +56,7 @@ func TestAccContainerRegistryTokenPassword_update(t *testing.T) {
 	data := acceptance.BuildTestData(t, "azurerm_container_registry_token_password", "test")
 	r := ContainerRegistryTokenPasswordResource{Expiry: time.Now().Add(time.Hour)}

-	data.ResourceTest(t, r, []acceptance.TestStep{
+	data.ResourceTestIgnoreRecreate(t, r, []acceptance.TestStep{


These tests are called _update so presumably they're checking that certain properties are updating correctly on the resource and not triggering a ForceNew, which means that this isn't a proper fix for the test and is masking an underlying issue.

The reason can be found at:

terraform-provider-azurerm/internal/services/containers/container_registry_token_password_resource.go

Lines 59 to 66 in cd57967

"expiry": {

Type: pluginsdk.TypeString,

Optional: true,

// TODO: Remove the force new and add customize diff to SetNewComputed on the `value` once https://github.com/hashicorp/terraform-plugin-sdk/issues/459 is addressed.

ForceNew: true,

ValidateFunc: validation.IsRFC3339Time,

DiffSuppressFunc: suppress.RFC3339Time,

},

Due to this bug, we are not able to update it, but can only recreate a new token password. This doesn't make a big difference for this resource.

Keeping the test called _update is misleading then since we're not actually updating anything, either the config should be adjusted so that we're actually correctly testing the update of update-able properties, or the test should be renamed or removed since we don't typically test for ForceNew behaviours

Agreed. I skip these two test cases now until we migrated to fw.

stephybun · 2024-11-11T08:48:17Z

internal/services/containers/container_registry_token_password_resource_test.go

@@ -86,7 +86,7 @@ func TestAccContainerRegistryTokenPassword_updateExpiryReflectNewValue(t *testin
 	data := acceptance.BuildTestData(t, "azurerm_container_registry_token_password", "test")
 	r := ContainerRegistryTokenPasswordResource{}

-	data.ResourceTest(t, r, []acceptance.TestStep{
+	data.ResourceTestIgnoreRecreate(t, r, []acceptance.TestStep{


The same applies to this test.

stephybun · 2024-11-12T10:52:32Z

internal/services/containers/container_registry_token_password_resource_test.go

@@ -56,7 +56,7 @@ func TestAccContainerRegistryTokenPassword_update(t *testing.T) {
 	data := acceptance.BuildTestData(t, "azurerm_container_registry_token_password", "test")
 	r := ContainerRegistryTokenPasswordResource{Expiry: time.Now().Add(time.Hour)}

-	data.ResourceTest(t, r, []acceptance.TestStep{
+	data.ResourceTestIgnoreRecreate(t, r, []acceptance.TestStep{


Keeping the test called _update is misleading then since we're not actually updating anything, either the config should be adjusted so that we're actually correctly testing the update of update-able properties, or the test should be renamed or removed since we don't typically test for ForceNew behaviours

This reverts commit 90476b1.

stephybun

Thanks @magodo LGTM 👍

github-actions · 2024-12-18T02:15:32Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

magodo added bug service/container-registry labels Aug 29, 2024

github-actions bot added the size/M label Aug 29, 2024

rcskosir mentioned this pull request Aug 29, 2024

ACR Token resource causing planning issues after recreation of the container registry #27215

Closed

1 task

Update

d89b363

magodo force-pushed the acr_token_password_markasgone branch from 247ffdf to d89b363 Compare September 10, 2024 12:09

stephybun requested changes Oct 30, 2024

View reviewed changes

internal/services/containers/container_registry_token_password_resource.go Outdated Show resolved Hide resolved

github-actions bot added the waiting-response label Oct 30, 2024

Update internal/services/containers/container_registry_token_password…

6156283

…_resource.go Co-authored-by: stephybun <steph@hashicorp.com>

github-actions bot removed the waiting-response label Nov 2, 2024

stephybun requested changes Nov 4, 2024

View reviewed changes

github-actions bot added the waiting-response label Nov 4, 2024

magodo added 2 commits November 11, 2024 10:55

Merge remote-tracking branch 'upstream/main' into acr_token_password_…

0100768

…markasgone

Merge main and pass tests

cd57967

magodo requested review from katbyte and a team as code owners November 11, 2024 03:04

github-actions bot removed the waiting-response label Nov 11, 2024

stephybun requested changes Nov 11, 2024

View reviewed changes

github-actions bot added the waiting-response label Nov 11, 2024

stephybun requested changes Nov 12, 2024

View reviewed changes

rename test cases

90476b1

github-actions bot removed the waiting-response label Nov 12, 2024

magodo added 2 commits November 13, 2024 10:11

Revert "rename test cases"

8d3f369

This reverts commit 90476b1.

Skip the test cases

d807b05

stephybun approved these changes Nov 15, 2024

View reviewed changes

stephybun merged commit 3c0a43d into hashicorp:main Nov 15, 2024
31 checks passed

github-actions bot added this to the v4.11.0 milestone Nov 15, 2024

stephybun added a commit that referenced this pull request Nov 15, 2024

Update for #27232

23d08f8

github-actions bot locked as resolved and limited conversation to collaborators Dec 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`azurerm_container_registry_token_password` - Handle gone container registry token and mark this password as gone #27232

`azurerm_container_registry_token_password` - Handle gone container registry token and mark this password as gone #27232

magodo commented Aug 29, 2024 •

edited

Loading

stephybun left a comment

magodo commented Nov 11, 2024

stephybun Nov 11, 2024

magodo Nov 11, 2024

stephybun Nov 12, 2024

magodo Nov 12, 2024

stephybun Nov 11, 2024

stephybun Nov 12, 2024

stephybun left a comment

github-actions bot commented Dec 18, 2024

	"expiry": {
	Type: pluginsdk.TypeString,
	Optional: true,
	// TODO: Remove the force new and add customize diff to SetNewComputed on the `value` once https://github.com/hashicorp/terraform-plugin-sdk/issues/459 is addressed.
	ForceNew: true,
	ValidateFunc: validation.IsRFC3339Time,
	DiffSuppressFunc: suppress.RFC3339Time,
	},

azurerm_container_registry_token_password - Handle gone container registry token and mark this password as gone #27232

azurerm_container_registry_token_password - Handle gone container registry token and mark this password as gone #27232

Conversation

magodo commented Aug 29, 2024 • edited Loading

Community Note

Description

PR Checklist

Changes to existing Resource / Data Source

Testing

Change Log

Related Issue(s)

stephybun left a comment

Choose a reason for hiding this comment

magodo commented Nov 11, 2024

stephybun Nov 11, 2024

Choose a reason for hiding this comment

magodo Nov 11, 2024

Choose a reason for hiding this comment

stephybun Nov 12, 2024

Choose a reason for hiding this comment

magodo Nov 12, 2024

Choose a reason for hiding this comment

stephybun Nov 11, 2024

Choose a reason for hiding this comment

stephybun Nov 12, 2024

Choose a reason for hiding this comment

stephybun left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 18, 2024

`azurerm_container_registry_token_password` - Handle gone container registry token and mark this password as gone #27232

`azurerm_container_registry_token_password` - Handle gone container registry token and mark this password as gone #27232

magodo commented Aug 29, 2024 •

edited

Loading