-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
Fix diagnostic & avoid variable override via environment #36435
Conversation
7f3699d
to
7a52518
Compare
e2959c5
to
3c3d096
Compare
3c3d096
to
e73ac99
Compare
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 this is going to be OK, and will help users going forward, but we should still be on the lookout for any existing workflows which might be impacted.
I don't think we're planning on any more v1.10 releases, so we might not want that backport label |
I don't have strong opinion but I assume that it's technically a regression, i.e. this worked fine in 1.9 and we (using your words 😄) broke it in 1.10 so I also concluded it would be good to fix that. We could also nudge people to upgrade I guess but we have not even cut a final 1.11 yet. Either way the backport can be sorted out post-merge. |
Fixes #36410
It turns out the ordering of variable source evaluation is correct, it's just the diagnostic message that is wrong due to mixed ordering of arguments to
fmt.Sprintf
.While the original report in #36410 suggests overriding variables via environment variables to be silently ignored, I think we cannot just make the same argument as was made in #36180
Specifically - We cannot expect users to be deleting autoloadable tfvars files. I do think it's reasonable to expect them to unset environment variables between plan and apply. Also, more importantly we only raise the diagnostic when values differ anyway.
That said after some consideration I chose to propose a warning that is still somewhat ignorable, just possibly noisy. I believe that is a good compromise that will not break workflows but still highlight what we consider anti-pattern.
Target Release
1.11.x
CHANGELOG entry