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

Deprecated argument messages contain no context for which property is deprecated. #1094

Open
RealOrangeOne opened this issue Nov 8, 2022 · 3 comments
Labels
bug Something isn't working

Comments

@RealOrangeOne
Copy link

SDK version

N/A

Relevant provider source code

N/A

Terraform Configuration Files

N/A

Debug Output

N/A

Expected Behavior

When an argument is marked as deprecated, it should be obvious which attribute is deprecated. Perhaps with the message "Argument foo is deprecated".

Actual Behavior

Unless the deprecation message itself contains a reference to which argument is deprecated (ie "The property foo is deprecated"), it's not obvious which field is deprecated. The SDK documentation examples don't mention to include the field name, and the example messages definitely don't.

Steps to Reproduce

References

Example output:

╷
│ Warning: Argument is deprecated
│ 
│   with module.xxxxxxxxx.checkly_check_group.project_group,
│   on modules/xxxxxxxxxx/group.tf line 1, in resource "checkly_check_group" "project_group":
│    1: resource "checkly_check_group" "project_group" {
│ 
│ This property is deprecated and it's ignored by the Checkly Public API. It will be removed in a future version.
│ 
│ (and 21 more similar warnings elsewhere)

The example here comes from the Checkly provider, but I believe this to be something better solved at the SDK level.

@bflad
Copy link
Member

bflad commented Nov 8, 2022

Partially related: hashicorp/terraform#31575

@bflad
Copy link
Member

bflad commented Nov 8, 2022

Hi @RealOrangeOne 👋 Thank you for reporting this and sorry you ran into trouble here. Out of curiosity, is this something occurring in a recent Terraform version and is the configuration that generates this warning referencing the deprecated attribute, but not configuring it? Normally Terraform core should be showing the source line in the diagnostic, if the issue is caused by a configured attribute value.

@bflad
Copy link
Member

bflad commented Nov 8, 2022

Ah ha, its an attribute under a set, which is why Terraform core is not showing the configuration source as it normally would. https://github.com/checkly/terraform-provider-checkly/blob/9328934dd9575d0b2af19d7f643cb9e0d5306461/checkly/resource_check_group.go#L209

This is a trickier case because the protocol between Terraform and providers does not currently support sets in diagnostic paths: https://github.com/hashicorp/terraform/blob/a574e95dcde2d3bbb399b025f515ccd062abeeca/docs/plugin-protocol/tfplugin5.3.proto#L43-L56 so a few things would require updates to properly have Terraform show or approximate the configuration source for this.

A tactical fix in this very specific case would be having the top level alert_settings attribute that is currently Type: schema.TypeSet and MaxItems: 1 switched to Type: schema.TypeList, since lists should work correctly with Terraform's diagnostic path mechanism and a set block with one item is functionally equivalent to a list block with one item. Terraform should then start showing the configuration lines that raised the diagnostic afterwards.

More generally though, having the SDK output include the attribute path information in the diagnostic summary/detail is problematic outside top level attributes as it needs to be done in a manner that is understandable by both practitioners and provider developers. There is no configuration reference equivalent for indexing into a set, for example, as set types in Terraform are not considered indexable. terraform-plugin-framework takes an approach of stringifying the path by wrapping the whole value in its own syntax, e.g. it would be something like alert_settings[Value(escalation_type=XXX,run_based_escalation=XXX,time_based_eascalation=XXX,reminders=XXX,ssl_certificates=XXX)].ssl_certificates which is likely difficult to understand in its own right. Taking a brief look at the SDK validation code it appears the built path where this rule would trigger approximates sets with list integer-based indexing which would translate to something like alert_settings[0].ssl_certificates. It may be fine in simpler set configurations, but it also may not wholly be accurate in relation to the placement of the set element in the actual configuration as the SDK does a lot of varied gymnastics with values after they come across the protocol.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants