Skip to content

Fix ImportResourceState RPC response decoding for identity data #36806

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

Merged
merged 4 commits into from
Apr 1, 2025

Conversation

austinvalle
Copy link
Member

cc @dbanck

This PR fixes a bug in the most recent v1.12.0-alpha20250319 release (and used alongside the changes in #36703) when importing via resource identity.

Currently, the resource identity returned by ImportResourceState is being decoded with the resource's main schema instead of the identity schema, which will give you an error if they don't match, like:

examplecloud_thing.test: Preparing import... [identity=cty.ObjectVal(map[string]cty.Value{"id":cty.StringVal("id-123"), "name":cty.StringVal("austin")})]

Planning failed. Terraform encountered an error while generating this plan.

╷
│ Error: ["name"]: unsupported attribute
│ 

If you'd prefer to just fix this in #36703 (or if there is more to fix), feel free to just close this PR 👍🏻. I mostly just opened this so I could move forward 😆


Target Release

1.12.x

CHANGELOG entry

N/A

@austinvalle austinvalle requested a review from a team as a code owner March 31, 2025 22:26
@austinvalle austinvalle added the no-changelog-needed Add this to your PR if the change does not require a changelog entry label Mar 31, 2025
Copy link
Member

@dbanck dbanck left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks for catching this and your contribution.

I would like to have a test case for this, something like TestGRPCProvider_ImportResourceIdentity. Is this something you would be interested in contributing or should I add it to this PR?

@austinvalle
Copy link
Member Author

Sure I can add that test here in a minute 👍🏻

@austinvalle
Copy link
Member Author

Added a test to both protocol versions in 0d9a30e

I also adjusted the mock schema so that the identity schema is different than the resource schema + updated the upgrade test that relied on that schema 👍🏻

@austinvalle
Copy link
Member Author

austinvalle commented Apr 1, 2025

Hmm, the CI failed on a test that I can't reproduce on my local machine 🤔, seems unrelated to the changes I made here:

--- FAIL: TestUiHookPreApply_periodicTimer (3.01s)
    hook_ui_test.go:148: Output didn't match.
        Expected: "test_instance.foo: Modifying... [id=test]\ntest_instance.foo: Still modifying... [id=test, 00m01s elapsed]\ntest_instance.foo: Still modifying... [id=test, 00m02s elapsed]\ntest_instance.foo: Still modifying... [id=test, 00m03s elapsed]\n"
        Given: "test_instance.foo: Modifying... [id=test]\ntest_instance.foo: Still modifying... [id=test, 00m01s elapsed]\ntest_instance.foo: Still modifying... [id=test, 00m02s elapsed]\n"
FAIL

@jbardin
Copy link
Member

jbardin commented Apr 1, 2025

time-based tests are notoriously flaky and need to be improved, re-running that will probably pass

Copy link
Member

@dbanck dbanck left a comment

Choose a reason for hiding this comment

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

Thanks for adding the test!

@dbanck dbanck merged commit 5c4c669 into hashicorp:main Apr 1, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog-needed Add this to your PR if the change does not require a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants