test: fixes all tests with granular provisioner steps #21065
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
b52ec3e to
d3ae796
Compare
03de57d to
4a270f4
Compare
4a270f4 to
98ad5d4
Compare
d3ae796 to
f1bb408
Compare
f1bb408 to
2b7bcfc
Compare
98ad5d4 to
3c9ea20
Compare
2b7bcfc to
64b9523
Compare
3c9ea20 to
3b5d651
Compare
64b9523 to
b1ae149
Compare
3b5d651 to
04bc86a
Compare
32cab2f to
6edb39c
Compare
da8b8e2 to
1ef7c09
Compare
spikecurtis
left a comment
There was a problem hiding this comment.
At a high level, I find the echo.Responses initializations confusing now, because they often include only a subset of responses of commands that will be run.
There is a lot of spooky copying from one response type to another in TarWithOptions , like from apply to graph, but also graph to apply?, and many others.
I think that either the tests need to be explicit about the all the responses, or we need some new abstraction that allows you to ergonomically setup the echo (e.g. a builder).
| } | ||
| } | ||
| } | ||
| `, |
There was a problem hiding this comment.
Why is this required now when it wasn't before? Wasn't terraform init implicit in configuring the session? I don't get what's changed.
There was a problem hiding this comment.
No it was not. configure did not run terraform init. So parse happened before init.
Meaning you could have Parsed a template that would fail terraform init before (and it did fail).
Now Parse happens after init, which imo makes more sense to validate with the terraform cli, and not our hcl parser.
These tests however are affected, they are now heavier, running the terraform executable.
There was a problem hiding this comment.
I don't like running a real terraform init because it causes the test to download the terraform provider from the internet, which increases flakiness. We just had a load of tests fail like that last night.
If parse is expecting some init results now, we should fake them in these tests.
1ef7c09 to
f06a77a
Compare
Yes the copying existed before to set sane defaults from what I could tell. If an Apply was given, then
A builder would be nice, I never liked the current pattern as the types are so deeply nested. Some I will make a PR in this stack where I see how much work it is to explicitly set all responses across all these tests... |
No description provided.