terraform icon indicating copy to clipboard operation
terraform copied to clipboard

configs: Remove unused `*Parser.LoadValuesFile`

Open alisdair opened this issue 2 years ago • 3 comments

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

Target Release

1.8.x

alisdair avatar Feb 22 '24 20:02 alisdair

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: https://github.com/hashicorp/terraform/pull/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. :thinking:

(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. :man_shrugging: )

apparentlymart avatar Feb 22 '24 20:02 apparentlymart

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:

https://github.com/hashicorp/terraform/blob/268c8e264d22604f2a4ec516ed4df7a5bdef421f/internal/command/meta_vars.go#L277-L285

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

alisdair avatar Feb 23 '24 18:02 alisdair

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.

apparentlymart avatar Feb 23 '24 20:02 apparentlymart

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

github-actions[bot] avatar Feb 26 '24 15:02 github-actions[bot]

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[bot] avatar Mar 28 '24 02:03 github-actions[bot]