Skip to content
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

Deprecation Warning For Variables #36079

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

DanielMSchmidt
Copy link
Collaborator

Companion PR to #36016, should give us a warning for deprecated variables both when used on the root module and on child modules. With child modules it shows the input in the module call in the context, with root modules it shows the deprecation attribute of the variable in the context.

Screenshot 2024-11-22 at 15 42 57

Fixes #18381

Target Release

1.11.x

Draft CHANGELOG entry

NEW FEATURES | UPGRADE NOTES | ENHANCEMENTS | BUG FIXES | EXPERIMENTS

@DanielMSchmidt DanielMSchmidt requested a review from a team as a code owner November 22, 2024 14:44
Copy link
Member

@jbardin jbardin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should probably be an error to have Deprecated set if there is no default value, since that is what makes the variable required.

@@ -309,6 +309,15 @@ func (n *nodeModuleVariable) evalModuleVariable(ctx EvalContext, validateOnly bo
finalVal, moreDiags := prepareFinalInputVariableValue(n.Addr, rawVal, n.Config)
diags = diags.Append(moreDiags)

if n.Config.DeprecatedSet && givenVal.IsWhollyKnown() && !givenVal.IsNull() && validateOnly {
Copy link
Member

@jbardin jbardin Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If validateOnly is true, then givenVal may likely be unknown (unless maybe it's a static literal in the config). Validation is done with as many values unknown as possible since it should work for all possible input values, and to get the most complete evaluation possible of all branches in the configuration

If you look above however (L295), is appears that cty.NilVal is used here to represent an entirely unset input variable (due to the legacy ability to assign null as a value to override module inputs), so simple direct comparison to that should suffice here.

@@ -92,6 +92,15 @@ func (n *NodeRootVariable) Execute(ctx EvalContext, op walkOperation) tfdiags.Di
}
}

if n.Config.DeprecatedSet && givenVal.Value.IsWhollyKnown() && !givenVal.Value.IsNull() && op == walkPlan {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly here, you should be able to determine if the variable is set by it being non-null. It's unfortunate that we have to wait until plan however.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecation Warning For Input/Output Variables
3 participants