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 3 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 this
TestStep
mode worry about a configuration? What happens if the configuration changes during a refresh step from a prior step? Here are the potential issues I can see here with a new/updated configuration:The prior
TestStep
with a configuration would've already caught the first few and I think rather than introduce the potential for these classes of issues, we can avoid this by not resetting the working directory and its configuration. The validation that a refresh step is not the first step can be implemented in thetestcase_validate.go
and/orteststep_validate.go
files, which runs prior to executing any of the test steps.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 refactored accordingly.
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.
Similar to the above, I'm not sure we should intentionally run
init
again, given the potential issues it could cause.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 refactored.
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
.