Skip to content

ImportStateVerify should apply DiffSuppressFuncs #103

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

Open
danawillow opened this issue Apr 6, 2018 · 3 comments
Open

ImportStateVerify should apply DiffSuppressFuncs #103

danawillow opened this issue Apr 6, 2018 · 3 comments
Labels
bug Something isn't working subsystem/tests Issues and feature requests related to the testing framework.

Comments

@danawillow
Copy link
Contributor

ImportStateVerify tests import a resource with a given id, and then check all fields in the imported state against an existing resource. However, it does these checks with strict equality, meaning that if a diff would have been suppressed, it still shows up in these tests as a diff. It would be great to be able to suppress those automatically so we don't have to complicate our test logic around checking those fields.

@hashibot hashibot transferred this issue from hashicorp/terraform Sep 26, 2019
@hashibot hashibot added bug Something isn't working testing labels Oct 2, 2019
@paddycarver paddycarver added subsystem/tests Issues and feature requests related to the testing framework. and removed testing labels Dec 17, 2020
@bflad bflad unassigned kmoe Feb 17, 2022
@bflad
Copy link
Contributor

bflad commented Feb 17, 2022

The next version of terraform-plugin-sdk/v2, unreleased at the moment, but slated to be v2.11.0 adds an additional DiffSuppressOnRefresh field to helper/schema.Schema. When enabled, this checks the DiffSuppressFunc during refresh. If the planned value matches the old state value via the DiffSuppressFunc, the old value will be preserved (or put another way, the planned value will be ignored). This should help mitigate cases like these where the DiffSuppressFunc implementations were likely put in place with the expectation that this would already happen in the SDK, however it previously did not.

We expect DiffSuppressOnRefresh to be generally safe to apply in most cases where DiffSuppressFunc is already being used, however it is not enabled by default for backwards compatibility.

The DiffSuppressOnRefresh functionality may also resolve this particular issue, but verification is required and I don't have a quick reproduction handy, so I'm hesitant to immediately close this out.

@apparentlymart
Copy link
Contributor

I'm not entirely sure that DiffSuppressOnRefresh can help in this particular case, because when we're importing we don't start with a configuration and so we don't have a distinct "original value" to preserve. If the import implementation and the refresh implementation disagree about how a particular value is normalized then DiffSuppressOnRefresh would preserve the form the import produced, but since importing and refreshing are typically based on the same API calls anyway I would expect that they would be identically-normalized in most cases.

With that said, I may be misunderstanding the original problem statement here. I think I've never written tests for an import function in particular and so I'm not sure exactly how that test process behaves; perhaps there's something extra going on for that particular testing situation which changes the circumstances so that DiffSuppressOnRefresh could be more helpful. 🤔

@danawillow
Copy link
Contributor Author

I filed this FR a while back so I don't have all the same context I did back then, but from what I remember: the Google provider uses ImportStateVerify basically as a way to check that we were writing every field we read from the API back to state. If we didn't, then the test would fail because the "imported" resource wouldn't have the field (since import calls read), but the state would (since it was in the config).

As a contrived simple example, let's say we have a string field, and we don't care about case. In a real life situation, the config could have field = "foo", the API could return field = "FOO", and a DiffSuppressFunc could ignore the case and make this not diff. In a test though, this same situation would show a diff and fail the test because the DiffSuppressFunc doesn't get run. This could be worked around by putting field = "FOO" in the test, but in a more complex situation, it wouldn't match user behavior as well.

Also cc @rileykarson who can probably talk about whether this is still a problem and may even be able to give real examples of trying to work around this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working subsystem/tests Issues and feature requests related to the testing framework.
Projects
None yet
Development

No branches or pull requests

7 participants