Conversation
If enabled, strict mode treats each warning as an error.
|
The equivalence tests will be updated. Please verify the changes here. |
|
The equivalence tests will be updated. Please verify the changes here. |
|
The equivalence tests will be updated. Please verify the changes here. |
|
The equivalence tests will be updated. Please verify the changes here. |
|
The equivalence tests will be updated. Please verify the changes here. |
dsa0x
left a comment
dsa0x
left a comment
There was a problem hiding this comment.
Nice work!. I have some comments around what kind of warnings we would be "promoting".
internal/moduletest/graph/apply.go
Outdated
| if ctx.Strict() { | ||
| setVariableDiags = moduletest.PromoteWarningsToErrors(setVariableDiags) | ||
| } |
There was a problem hiding this comment.
Taking another look at FilterVariablesToModule, it no longer returns any diagnostic even, so we can update that function, and this entire logic would not even be relevant.
internal/moduletest/graph/apply.go
Outdated
| if ctx.Strict() { | ||
| unexpectedDiags = moduletest.PromoteWarningsToErrors(unexpectedDiags) | ||
| } | ||
|
|
There was a problem hiding this comment.
I think we should be doing this outside this function. This function should be for the expected_failures conversion, and that is not related to strict. Before passing the original diagnostics to this function, we can apply "strictness" to them.
| if ctx.Strict() { | ||
| variableDiags = moduletest.PromoteWarningsToErrors(variableDiags) | ||
| } |
There was a problem hiding this comment.
It may be important to clarify what kind of warnings we are turning into an error. In my opinion, the warnings here are not about the operation made by the run, but by declaring an unused variable within the test, and I would argue that should not cause the test to fail, even in strict mode. The warning we should be focused on are the ones produced by the test's execution of a terraform operation (plan/apply)
There was a problem hiding this comment.
Hey @garvitarai1 would you mind chiming in here?
The original Issue is this one .
We wonder what should constitute the strict mode - whether we should report every warning as an error (similar to some other toolings in other languages) or we should be selective with promotion of warnings to errors to the scope Sams proposed in the message above?
And the more important question is - does having one datapoint about the nice-to-have strict mode in "discuss" 2.5 years ago warrants introducing this mode?
There was a problem hiding this comment.
I checked Jira and quickly scanned GH issues as well, so this seems like a one off request. I have the same instinct as Sams that not all warnings are equal, and this might be a noisy experience to promote all warnings to errors.
I think the path forward depends on where the implementation stands. If the work is done, then I'd vote for shipping this as an experiment in an alpha release. In this case, let's bring this to the team meeting to get more opinions on the behaviour. If it needs significant effort, then lets de-prioritize.
|
The equivalence tests will be updated. Please verify the changes here. |
We've stopped returning diags from the above method, and so we don't need to populate the named return value with nils all the time, it's better to remove it.
|
The equivalence tests will be updated. Please verify the changes here. |
3 participants
TBD
Fixes #
Target Release
1.15.x
Rollback Plan
Changes to Security Controls
Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.
CHANGELOG entry