hashicorp / terraform

Terraform enables you to safely and predictably create, change, and improve infrastructure. It is a source-available tool that codifies APIs into declarative configuration files that can be shared amongst team members, treated as code, edited, reviewed, and versioned.
https://www.terraform.io/
Other
42.53k stars 9.52k forks source link

Deprecation Warning For Input/Output Variables #18381

Open jonbrouse opened 6 years ago

jonbrouse commented 6 years ago

As open sources modules become more popular and teams in organizations create Terraform modules that depend on the remote state of another team's module, there is a need to communicate the evolution of a module.

This request is proposing the addition of an optional deprication_warning parameter to the output and input variable configuration. The depreciation notification would display when a module references the deprecated output via remote state.

Terraform Configuration Files Example

For this example we will first view the outputs.tf file from v1.0.0 of an example module.

output "route53_zone_id" {
  value = "${aws_route53_zone.route53_zone.zone_id}"
}

Below is the outputs.tf file from v1.1.0 of our example module. The output variable route53_zone_id has been updated to primary_zone_id.

output "route53_zone_id" {
  value = "${aws_route53_zone.primary_zone.zone_id}"
  deprication_warning = "The route53_zone_id output variable has been deprecated. Please use primary_zone_id"
}

output "primary_zone_id" {
  value = "${aws_route53_zone.primary_zone.zone_id}"
}

Expected Behavior

When the apply is run in the root module that references the output, a depreciation warning would be displayed

Warning: The route53_zone_id output variable has been deprecated. Please use primary_zone_id

Another option would be to use deprication_replacement instead of deprication_warning. This would allow Terraform to have more control over the message.

output "route53_zone_id" {
  value = "${aws_route53_zone.primary_zone.zone_id}"
  deprication_replacement = "primary_zone_id"
}

Output of terraform apply

Warning: module.our_example: "route53_zone_id": [DEPRECATED] Use primary_zone_id instead
apparentlymart commented 6 years ago

Hi @jonbrouse! Thanks for sharing this use-case.

Being able to mark things as deprecated definitely seems like a good idea. For outputs in particular things are tricky though, because Terraform doesn't currently analyze their usage in a way that would permit the conditional warning you suggest here. That doesn't mean it's impossible, of course, but we'll need to think about the best way to achieve it within Terraform's internal architecture, and probably also to accept that the warning won't be displayed in some edge cases where the attribute is accessed dynamically.

Supporting this idea for variable blocks would be easier, I think, because we're already verifying statically that each of the arguments in a module block correspond to a variable block with suitable settings.

We generally lean towards shorter names for these built-in arguments inside variable and output blocks, so I'd lean towards naming the argument merely deprecated to keep things concise:

output "route53_zone_id" {
  value      = "${aws_route53_zone.primary_zone.zone_id}"
  deprecated = "Use primary_zone_id instead."
}

output "primary_zone_id" {
  value = "${aws_route53_zone.primary_zone.zone_id}"
}

With the new warning message style coming in the next major release, that might generate something like this:

Warning: Deprecated output value

  on example.tf line 21, in resource "aws_route53_record" "example":
  21:   zone_id = module.example.route53_zone_id

The output value route53_zone_id is deprecated. Use primary_zone_id instead.

The development branch for the next major release has some significant changes to the configuration loader already, so we'll need to hold on this for now at least until that work is completed and merged. We'll consider this some more for a subsequent release.

Thanks again for this suggestion!

jonbrouse commented 6 years ago

Hey @apparentlymart thanks for the reply. Y'all are doing awesome work!

jasonmcintosh commented 6 years ago

Assuming Hashicorp is considering adding support for deprecating parameters - are you looking to add it on variables as well? Be nice to support "module version 1.2.2 deprecates this variable, instead use X to be removed in 1.3" kinda thing :)

slavaaaaaaaaaa commented 5 years ago

I'm seeking this as well, more for general use than just outputs.

In my case, I'm deprecating an internal company module in favour of including its resources in another module. Procedures for this require state moves, so embedding such instructions in the to-be-deprecated module would be ideal.

rodrigodiez commented 4 years ago

I am also interested in this behaviour. As we evolve our infrastructure we have to change our modules. Being able to warn engineers about deprecations is a key feature to help them migrate their code base gradually

oswalya commented 3 years ago

I would also be interested in seeing this. Any update?

TrueCyberAxe commented 3 years ago

Any progress?

ghostsquad commented 3 years ago

Is this on the roadmap?

bryantbiggs commented 2 years ago

I hate to be the +1 guy but bumping for visibility and maybe Hashicorp's current thoughts on this enhancement 🙏🏽

apparentlymart commented 2 years ago

As far as I'm aware, there is not yet any clear design for how to reliably determine when referring to a particular output value in a calling module, and I believe solving that would be the main blocker for implementing something like this.

Using the names from the original writeup as a placeholder, module.example.route53_zone_id is a straightforward case that I think we could handle today, but for a module using for_each we may see module.example["foo"].route53_zone_id which requires a more detailed level of static analysis than Terraform has needed before.

It's also possible to use an entire module instance as a value and pass it around as an object, which would require even more sophisticated static analysis to resolve fully. For example:

module "example" {
  # ...
}

locals {
  intermediate = {
    for a, v in module.example : a => v if contains(["route53_zone_id", "domain_name", a])
  }
}

output "zone_id" {
  value = local.intermediate.route53_zone_id
}

If is of course debatable how many of these more complicated cases are important to handle, but deciding that will itself require research, and so will be a part of formulating a design for this.

bryantbiggs commented 2 years ago

I wonder if there is a more generic way that this could be handled. a construct like the move block that has been added, where users can simply provide a deprecation notice and its up to them to provide the text. I agree that with the deeply nested constructs of for_each maps and count it can be hard to tell whats what, but I can see a very clear use case today where I simply want to output a notice of pending changes in a plan/apply. I guess this also raise another question of verbosity as well in terms of plan/apply outputs

md5 commented 2 years ago

It this issue about deprecation of outputs only or variables as well? I see that #19138 has been marked as a duplicate of this issue, but that issue is about variables, not outputs. What we're interested on the team I work with is marking inputs as deprecated, not so much outputs.

I feel like the two issues are separate (as is deprecation of modules), but if this issue is about both it seems like the description should be updated to reflect that.

j-flat commented 2 years ago

It this issue about deprecation of outputs only or variables as well? I see that #19138 has been marked as a duplicate of this issue, but that issue is about variables, not outputs. What we're interested on the team I work with is marking inputs as deprecated, not so much outputs.

I feel like the two issues are separate (as is deprecation of modules), but if this issue is about both it seems like the description should be updated to reflect that.

Like @md5 I would also be more interested in marking input variables as deprecated, where migrating from old input variables to new ones would be made more clear with these warnings.

quentin9696 commented 2 years ago

Hi,

We are also interest on that feature on the variables. Do you have any update on this?

Thanks for your great job!

pasathish commented 2 years ago

Hi, We are also interested in marking input variables as deprecated. Any updates on this?

InsanesTheName commented 1 year ago

Throwing my hat into the ring as well - I also would love to have this functionality (for both variables and outputs)!

crw commented 1 year ago

Just a reminder to please use the 👍 reaction on the original post to upvote issues - we do sort by most upvoted to understand which issues are the most important. This also reduces "noise" in the notification feed for folks following this issue. Thanks!

Unfortunately there are no other updates to share on this feature request. Please keep upvoting, we do look at that to help prioritize community requests.

rquadling commented 1 year ago

If consideration is ever given to this, one particular use case I'm having to deal with is how to advertise a deprecated root module output that would be accessed via the remote state provider of Terraform. This is in addition to module consumers.

We've found the remote state provider very useful in reducing the amount of lookups (and so performance improvements) when small projects need to deal with some infrastructure, and then share out the ID/ARNs or some elements in a structured way (for example, ALBs, what listeners are active and expected to be used, etc.).

It's not a major headache, but a simple "you are using a deprecated output" would be SO useful.

daodennis-ri commented 1 year ago

Does this issue cover deprecation warnings for output values or does it include inputs too?

tom-reinders commented 1 year ago

The closed as duplicate issue #19138 would make me assume it includes inputs too, but the title of #18381 issue makes it confusing

wzooff commented 5 months ago

I agree with @tom-reinders. And because I don't have a proper issue, I will post my workaround here.

locals {
  deprecated_variables = toset(compact([
    length(var.list_name) > 0 ? "list_name" : "",
    var.string_name != "" ? "string_name" : "",
  ]))
}

resource "null_resource" "deprecated_variable" {
  for_each = local.deprecated_variables
}

If someone used/specified deprecated variables, respective null_resources will be created.

It works in conjunction with CI on the consumer side. You can parse terraform plan and notify in PR (tested in Spacelift with notification/plan policies) that it would be nice to migrate from deprecated parameters. I have mentioned deprecations in the variable description as well.

jonbrouse commented 1 month ago

Updated the issue to explicitly include input and output variables.