Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Adding RefreshState test step #1070
Adding RefreshState test step #1070
Changes from 7 commits
f9a2ca4
204d99e
468b72d
a5fad91
e17526a
51342f5
2f726cb
0f220c8
4be309c
034a8b7
bc221f7
830445e
9ec2d62
5be3380
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should consider what website documentation we can add around this. It'd also be good to denote here and in the website that its current intention is refresh, (potentially) plan, and optionally running checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This field should also document that it cannot be the first
TestStep
and any other potential restrictions or contradictory definitions (such asImportState
+RefreshState
for example)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the documentation. In terms of the website docs, where do you think they should be added? The information around
ImportState
is in/plugin/sdkv2/resources
. As we're just discussing testing here should be add a new section or add/amend an area of the website docs that already exist?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think https://www.terraform.io/plugin/sdkv2/testing/acceptance-tests/teststep is okay enough for now. Ideally that page would probably be broken up by test mode, but adding information in the current information architecture should be okay. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have added a couple of lines. Let me know if you think this needs to be expanded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should refresh
TestStep
run plan after the refresh to check for unexpected plan differences? If so, it'll need that Terraform command added and check againstExpectNonEmptyPlan
if those differences are expected.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have added a plan following the check and the check against
ExpectNonEmptyPlan
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: It'd be nice if the provider resource actually converged after the apply without an unexpected plan difference so it didn't look like this sort of post-RefreshState ExpectNonEmptyPlan was required, but no worries if this is difficult to work out. I think we may be able to reset the time during the
Create
function by explicitly calling something like:But I'm not sure if that's the least confusing option there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Have used
PreConfig
to reset time forTestStep
to avoid state mutation inReadContext
and subsequent diff generation byCustomizeDiff
.