Allow variables to be marked as deprecated to communicate variable removal to module consumers

[reegnz]

OpenTofu Version

1.6.0-beta4

Use Cases

Over the lifecycle of an opentofu module, one might need to remove variables from a module.
It would be great to have a mechanism to notify users that they are using a deprecated variable that is scheduled for a future removal.

Attempted Solutions

• keep maintaining backward compatibility of your modules, never break clients (works a lot of the time)
• notify module users out of band using semver version bumps and release notes (difficult to pre-alert programmatically, major version bump results in instant abandonment of the module user if the bugfixes are not ported back to multiple release branches)
• build a deprecation solution based on comments - needs to fetch modules, parse hcl, be aware of terraform semantics of module variable usage, etc. too big task for me

Proposal

Introduce a new field into the variable declaration that is used to define a deprecation message:

variable "this_is_my_variable" {
  type = string
  description = "This is a variable for the old way of configuring things"
  deprecation = "This variable will be removed on 2024-12-31. Use that_is_my_variable instead".
}

Whenever a user is using a deprecated variable of the module, they will get a warning message on tofu validate, tofu plan and tofu apply invocations as long as the variable is still being passed to the module.

The deprecation message is freeform text, so it could contain a URL to migration instructions too.

References

Other languages support communicating deprecation to software component users.

• java has the @Deprecated annotation and even had @deprecated javadoc comments in Java 1.1, 26 years ago.
• go has a comment-based deprecation mechanism: https://zchee.github.io/golang-wiki/Deprecated/
• jsdoc for javascript/typescript also solves this in comments
• terraform providers already have a way to signal deprecations to users, it would make sense to make that feature a 1st class citizen for modules as well: https://developer.hashicorp.com/terraform/plugin/sdkv2/best-practices/deprecations

[reegnz]

Just noticed there's a 5 year old feature request for this in terraform: hashicorp/terraform#18381

[ghost]

Hi @reegnz this is a cool idea, I like it. However, I believe this would make module code incompatible with Terraform, which some module authors probably would have an issue with.

We are currently working towards a stable OpenTofu release, I think this proposal could be revisited after that release is out. I added a label to that effect.

[cam72cam]

I think it's a neat idea and would be useful, but share the same concern as @janosdebugs. We need to figure out how to handle that compatibility going forward. I wonder if a comment annotation would be a reasonable workaround, though it would definitely be a bit of a hack.

[stephen-dahl]

Just noticed there's a 5 year old feature request for this in terraform: hashicorp/terraform#18381

That issues title and comments specifically reference output values not variables. I think separate issues for these things is good since we know how to implement this for variables but still don't know how to handle outputs. Let variables be implemented then circle back to outputs when someone figures out a plan.

[ghost]

Maybe a good way to implement this would be to detect a deprecated line in the description field like this:

variable "this_is_my_variable" {
  type = string
  description = "This is a variable for the old way of configuring things. Deprecated: This variable will be removed on 2024-12-31. Use that_is_my_variable instead."
}

The rule could be that the deprecation notice must always be at the end of the description and must have the format of:

Deprecated: explanation.

OpenTofu could then extract this text and display it as a warning to the user. This would work for both variables and outputs and could be implemented in one go.

@reegnz what do you think?

[lorengordon]

I think parsing descriptions is bound to be flaky. I'd maybe suggest a custom "deprecation" provider, to maintain compatibility with Hashicorp Terraform.

[ghost]

@lorengordon how would that work?

[lorengordon]

Just the way providers work. Write a resource with the behavior and workflow you want. Something like, take the deprecated variable value and an deprecation message as an input, generate an error with the deprecation message if the variable value is not null. Or take inspiration from variable validation, and take a condition argument that let's the user decide when to trigger the error.

[ghost]

@lorengordon this wouldn't work in on outputs, and it also wouldn't work if the user passed in the default value of a variable explicitly. Both of these would trip up someone if the variable/output in question went away.

[lorengordon]

This issue is about variable deprecation, not outputs. That would be different anyway. Would probably want the warning to occur when a user accesses the deprecated output. That's going to require changes to terraform core, no matter what.

But the provider idea certainly would work if the user passed in the default value explicitly. The module author would be responsible for determining what happens in that scenario. There are options. The resource could also simply take another input for the default value, and decide how to handle the scenario when the value passed in matches the default. Or the condition expression could handle that itself, up to the author.

[Yantrio]

Now that the stable release has been out for some time we are happy to accept contributions on this issue. We feel it is valid.

In my personal opinion, I feel it may be best to ensure that we as a community agree on the approach before development starts though.

We should also be careful to make sure that we narrow down the scope to only variable blocks as described in the original issue.

[sanskruti-shahu]

Hey, I would like to take up this issue.

[ghost]

Hey @sanskruti-shahu thank you for volunteering, I've assigned you. Before you begin coding, please read this section of the contribution guide.

[cube2222]

Other than the above, I propose that we use a more "special string" than just "Deprecated: ".

E.g.

variable "this_is_my_variable" {
  type = string
  description = "This is a variable for the old way of configuring things. @deprecated: This variable will be removed on 2024-12-31. Use that_is_my_variable instead."
}

To display the warnings, I think you can just return them as part of the diags struct (as a warning) during the planning phase and they will be propagated up and displayed.

I don't think there needs to be any special apply-time handling of this.

[cam72cam]

Perhaps @deprecated{message} or some other enclosing mark?

[sanskruti-shahu]

I agree with @cube2222 , I will go through the codebase once and will let you know about changes I'll be making before implementing them.

[skyzyx]

@cam72cam said:

I think it's a neat idea and would be useful, but share the same concern as @janosdebugs. We need to figure out how to handle that compatibility going forward. I wonder if a comment annotation would be a reasonable workaround, though it would definitely be a bit of a hack.

I actually really like this idea, and if an update to the HCL parser would enable first-class support for features-by-comment, this could open some new doors. (I've used the HCL parser, but never for trying to read comments, so I don't recall if it's possible at-present.)

The current reality is that we live in a mixed ecosystem where people/orgs/companies will be spending the next few years transitioning tools (assuming they plan to come over to OpenTofu). If anyone remembers Mark Pilgrim (Dive into Mark, assassinated his digital presence in 2010), he wrote a really great maxim:

Be liberal in what you accept, but conservative in what you produce.

The ability to opt-in and enable new features that are not broadly compatible is interesting.

# @deprecated: This variable will be removed on 2024-12-31. Use that_is_my_variable instead.
variable "this_is_my_variable" {
  type = string
  description = "This is a variable for the old way of configuring things."
}

I think that a well-defined syntax for this would be in order.

`@` + KEYWORD + COLON + SPACE + DATA
# @deprecated: This variable will be removed on 2024-12-31.

And if we look at the pattern used by conventional commits, we can do a little more.

`@` + KEYWORD + [ `(` + ALT [ , ALT ] + `)` ] + COLON + SPACE + DATA
# @terratest(custom_param): abcd1234
# @spacelift(otherCustomParam): zyxw9876
# @other_community_tool(other-other-custom-param): lmnop654

I would caution, however, that we take the time to learn from the W3C's mistakes with -webkit, -moz, and -ms prefixes in CSS. Many vendors implemented things in slightly different ways, and web devs made the mistake of just -webkit-ing everything. (i.e., Don't @spacelift something if @env0, @terragrunt, or others plan to implement the same thing.)

I would hope that with a spec, we could get community + tooling support (across HashiCorp, the OTF, and others) for adding features via comments that could eventually make their way into the core HCL parser(s), like browser engines have.

Something like a @deprecated comment (experimental, incubating) could eventually graduate to a proper deprecated = "" property supported by all tools in the ecosystem.

I would love to be able to say — for tools like tofuenv and tenv — something like:

terraform {
  # @opentofu(required_version): ~> 1.6.0
  required_version = ">= 0.13, < 2.0"

  required_providers {
    # https://github.com/hashicorp/terraform-provider-aws/releases/latest
    aws = {
      source  = "hashicorp/aws"
      version = ">= 4.0, < 6.0"
    }
  }
}

tfenv and tfswitch would read required_version while tofuenv and tenv would first read @opentofu(required_version) if it exists, and if not, fall back to required_version. Nobody's HCL parser freaks out and crashes.

---

(CSS had it easy because browsers simply ignored things it didn't understand. The HCL parser throws errors/panics.)

[sanskruti-shahu]

Hey @skyzyx, I got your point but I think reading comment for deprecation message would be quite complex than directly checking out variable's description.

[cube2222]

Alright, we've discussed this internally, and have a hard time deciding between the two approaches:

1. Adding it as an explicit deprecated field in the variable block.
2. Adding it as part of the description.

The comment approach would most likely require us to for the HCL repo, which isn't something we want to do right now.

Thus, we've decided to submit this to the steering committee for decision. This might take a week or two to resolve, until the steering committee will meet.

[reegnz]

So until then the next best thing is to try and build the deprecation warnings into a plugin for tflint: https://github.com/terraform-linters/tflint/blob/master/docs/user-guide/plugins.md

[ghost]

Hey folks, we have discussed this with the core team and believe that this should only be done once #1328 is done.

[sftim]

I hope if we come up with a design, it also works for deprecating locals.

[cam72cam]

@sftim can you elaborate on deprecating locals? The main goal behind this issue is to communicate to consumers of a module that the inputs (and probably outputs) have changed are will not be supported in a future version.

[sftim]

Deprecation could look, for example, like this:

-locals {
-   foo = data.foo_bar.json_value
-   
-   foo_json = jsonencode(data.foo.value)
-}
+#[deprecated]
+locals {
+   # use local.foo_json instead
+   # the values are NOT equivalent, see JIRA-42
+   foo = data.foo_bar.json_value
+}
+locals {
+   foo_json = jsonencode(data.foo.value)
+}

Having two different deprecation mechanisms would be annoying to coders and to people explaining it all.

[sftim]

This could work, I think:

variable "this_is_my_variable" {
  lifecycle {
    deprecated = true
  }
  type = string
  description = "This is a variable for the old way of configuring things."
}

[cam72cam]

@sftim I want to understand why you want to deprecate locals? The main goal of this issue is to communicate to consumers of a module that the inputs have changes?

I'm also a fan of keeping the description the same and adding an additional "deprecation" field as discussed above.

[sftim]

When we design how to deprecate module inputs, we should consider how we'll mark other things duplicated (eg: provider-managed functions, locals, outputs, entire modules, inner modules).

For example, in Python you can mark any internal method as deprecated and gradually update your code. You can even lint on adding new uses of deprecated things.

[cam72cam]

Going back to your python example, that's still a "function call" interface. Are you looking for something that can hint to IDEs that a particular local in a module should eventually removed? Why wouldn't a comment suffice there?

The biggest value IMO of deprecated is at the module interface. Saying "This Input" or "This Output" won't be supported much longer provides clear messaging between distinct teams and organizations (module writers and maintainers vs consumers).

Given that locals are module-internal, I don't see the need for any of that information to be provided to consumers of the module. Any modern editor can easily and will happy identify and point out //TODO and //FIXME in code.

To make it more concrete, I would not want tofu warning me that a module I use has internal stuff that's deprecated. That would just be confusing to me. I would only want to know that I need to update my calling code when I use a module directly.

This does bring up a scenario on the original issue that we have yet to consider: Would you only want deprecated warnings from the root module -> first level child modules? Does it ever make sense to warn about child -> child module deprecation?

[sftim]

Imagine you are working on a module and use a deprecated detail of it. You get a warning. Making the warning machine readable and unambiguous is useful, even if IDEs can draw inferences.

The thing to watch out for is ending up with more than one way to mark things deprecated. If we're happy that won't happen, all is well.

[reegnz]

I agree it would be best to have a consistent deprecation mechanism across the terraform language. Languages like Java allow you to deprecate fields, methods, classes, interfaces etc the same way with annotations, and the compiler reports on deprecated code being used.

If we think holistically module user facing and module developer facing ways of deprecation should not be bifurcated. I still think initially the consumer-side should be worked on, unless the change is trivial enough to do it across the board at the same time.

Nowadays I think this should be solved outside the tool as a linting solution, via comments. Changing the language makes sense once it's proven this can be linted.
Changing the language also has the risk of the mechanism being too coupled to the language and hard to change.
Linting is not coupled to the language, although if there's multiple lining tools it runs the risk of splitting the community, module authors having to support multiple linter comment syntaxes, etc.

Anyway, maybe someone could contribute something to tflint for this purpose.

[ghost]

Hey @sftim thank you for your input. We discussed your suggestion with the core team and due to how the HCL language works, a generalized deprecation mechanism is difficult to implement. It is also not what this issue is about, so we would like to ask you to open a separate issue if you would like to see a generalized mechanism.

[sftim]

I'll try to clarify; I don't want to request a general mechanism in this issue. I do want the design to leave room for future work.

[ghost]

@sftim this is a topic that would likely require an RFC with technical details. HCL is a very restrictive language and the codebase is extremely complex and "designing for the future" isn't a concept that is easily applied here.

[jimbocoder]

I might have a good idea that maintains compatibility with Hashi, so module authors are not discouraged, but is also not too hacky.

Since the most recent versions allow for files with .tofu extensions, which Hashi will ignore, you could define a new block type, something like

deprecation "my_var" {
  type = "variable"
  message = "This variable is deprecated!"
}

deprecation "my_output" {
  type = "output"
  message = "The output called my_output is going away soon!"
}

If you only define these blocks in a file with the .tofu extension, but define the variables and outputs themselves in .tf files, it would work with either tool, so module authors wouldn't be shied away, and even encouraged to support tofu further since it makes their module better in a way that hashi can't.

[apparentlymart]

One interesting detail to consider here is exactly what in the calling module ought to trigger the deprecation warning.

The easiest case to call is directly assigning a non-null value to an optional input variable:

module "example" {
  source = "./example"

  deprecated_thing = "foo"
}

A less obvious case is when the expression being assigned produces a null value, which is how we represent the idea of making a dynamic decision about whether an input variable is being set:

module "example" {
  source = "./example"

  deprecated_thing = var.use_deprecated_thing ? "foo" : null
}

Should the mere presence of a definition of deprecated_thing be sufficient to generate the deprecation warning, or should it only generate a warning if the value assigned is not null?

Making an exception for null does give a module author a little more flexibility in how they might gradually phase out use of a deprecated input variable. However, once the variable "deprecated_thing" block is removed from ./example the above example would become invalid regardless of whether the final value of that expression is null or not, so hiding the warning when the value is null risks that a user of this module would not see the deprecation warning early enough to react to it.

I'd also note that when providers implement deprecation they do that "server-side" in the plugin protocol (where OpenTofu Core is acting as the client) and the plugin protocol does not allow a provider to distinguish between an omitted argument and an argument explicitly set to null, so the existing precedent for provider arguments is that a deprecation warning is not generated when a deprecated argument is explicitly set to null.

I think an RFC related to this feature request ought to take a position on this question and discuss both the advantages and disadvantages of that position.

---

@cam72cam's earlier thought about only generating warnings for the first level of module calls might be an interesting way to compromise here, since I think the most likely reason for writing a conditional assignment like I showed above is that someone is maintaining an intermediate module that bridges between two other modules where they want to give their users the option of continuing to use the deprecated feature without imposing a deprecation warning on calls that set var.use_deprecated_thing to false.

If we did that then I assume we'd want to make sure that the deprecation warnings appear when using tofu test, since shared modules are often never used as a root module and so if we don't return the warnings during testing the author might never get an opportunity to see the warning.

[sftim]

deprecation "my_var" {
  type = "variable"
  message = "This variable is deprecated!"
  condition = var.my_var != null
}

?

[ollevche]

Hey there! Feel free to leave your thoughts on the RFC for the deprecation of module variables and outputs: #2180