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

configs: Remove unused *Parser.LoadValuesFile #34716

Merged
merged 1 commit into from
Feb 26, 2024

Conversation

alisdair
Copy link
Contributor

While working in the area I noticed that this method is not used anywhere, so we can remove it.

Target Release

1.8.x

@apparentlymart
Copy link
Contributor

apparentlymart commented Feb 22, 2024

A funny coincidence that (as a background task while I was watching a presentation this morning) I just implemented something that could have been a caller of this function, if only I'd remembered that it existed: #34718 (the tfvarsdecode function, specifically).

I don't feel that what this function does is complex enough that we necessarily need to retain a separate function for it vs. just having equivalent behavior inline as I did in that PR, but since this coincidence happened I suppose it's worth thinking about which of the two approaches we'd find preferable here. 🤔

(Since Terraform CLI is able to load .tfvars files I assume there must be some other code like this function somewhere, and so perhaps the middle ground would be to factor out that other function into a more sharing-friendly place and then use it in tfvarsdecode function. 🤷‍♂️ )

@alisdair
Copy link
Contributor Author

My softly-held opinion: I think we're better off having separate implementations, and removing this one. The logic is both simple and divergent (for arguably-good reasons), so I don't think there's enough benefit to try to build a generic tfvars package.

The other implementation of related logic is in the command package, and you can see it's got one significant difference:

attrs, hclDiags := f.Body.JustAttributes()
diags = diags.Append(hclDiags)
for name, attr := range attrs {
to[name] = unparsedVariableValueExpression{
expr: attr.Expr,
sourceType: sourceType,
}
}

I can't remember why we delay evaluation of variable values, but I assume we won't want to change that at this time.

@apparentlymart
Copy link
Contributor

apparentlymart commented Feb 23, 2024

We delay because of the historical quirk that we parse the environment variables and -var command line options differently depending on the declared type of the variable, and so we need to wait until the configuration is resolved enough to make that decision. This was originally split awkwardly across the command, backend, and terraform packages but IIRC we later managed to refactor it so that Terraform CLI handles this problem completely, but it does still need to wait until it gets far enough to have loaded the configuration before turning this into a terraform.InputValues.

So indeed, it does seem like there's no commonality between these two cases: the hypothetical new function I added would have no benefit in delaying parsing in this way because it doesn't have any variable declarations to refer to anyway, and it's also always starting with HCL syntax and so doesn't need that extra nudge to sometimes skip parsing and just use the string value verbatim.

@alisdair alisdair merged commit 9c31e99 into main Feb 26, 2024
6 checks passed
@alisdair alisdair deleted the alisdair/remove-configs-parser-load-values-file branch February 26, 2024 15:20
Copy link
Contributor

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

Copy link
Contributor

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants