V2 DeprecationMessage missing configuration file and line context

[lafentres]

SDK version

{
  "Path": "github.com/hashicorp/terraform-plugin-sdk/v2",
  "Version": "v2.3.0"
}

Relevant provider source code

TFE provider data source with a DeprecationMessage

func dataSourceTFEWorkspaceIDs() *schema.Resource {
	return &schema.Resource{
		DeprecationMessage: "\"ids\": [DEPRECATED] Use full_names instead. The ids attribute will be removed in the future. See the CHANGELOG to learn more: https://github.com/hashicorp/terraform-provider-tfe/blob/v0.18.0/CHANGELOG.md",
		Read:               dataSourceTFEWorkspaceIDsRead,

		Schema: map[string]*schema.Schema{
...

Terraform Configuration Files

This uses a local build of the TFE provider because we haven't released the version with the V2 upgrade yet.

To see the expected behavior with V1 of the SDK, switch the provider version to 0.23.0.

terraform {
  required_providers {
    tfe = {
      source  = "hashicorp/tfe"
      version = "YOUR_LOCAL_BUILD_VERSION"
    }
  }
}

provider "tfe" {
  hostname = "app.terraform.io"
}

resource "tfe_organization" "test-org" {
  name  = "tst-sdkv2-org"
  email = "YOUR_EMAIL"
}

resource "tfe_workspace" "workspace-1" {
  name         = "workspace-1"
  organization = tfe_organization.test-org.name
}

resource "tfe_workspace" "workspace-2" {
  name         = "workspace-2"
  organization = tfe_organization.test-org.name
}

// Shhhh. Just ignore the depends_on. I needed a really easy example config for a PR
// I'm working on and this was the simplest way to make the config work for everyone
data "tfe_workspace_ids" "all" {
  names        = [tfe_workspace.workspace-1.name, tfe_workspace.workspace-2.name]
  organization = tfe_organization.test-org.name
  depends_on = [
    tfe_workspace.workspace-1,
    tfe_workspace.workspace-2,
  ]
}

Debug Output

Please let me know if you need this. I don't think this contains info that would help here but I'm not 100% sure.

Expected Behavior

Running terraform plan should have shown the deprecation message and the relevant configuration file and line context like it did in V1 of the SDK (which you can see by using v0.23.0 of the TFE provider)

Warning: "ids": [DEPRECATED] Use full_names instead. The ids attribute will be removed in the future. See the CHANGELOG to learn more: https://github.com/hashicorp/terraform-provider-tfe/blob/v0.18.0/CHANGELOG.md

  on main.tf line 117, in data "tfe_workspace_ids" "all":
 117: data "tfe_workspace_ids" "all" {

Actual Behavior

Running terraform plan showed the deprecation message but didn't include any information about the configuration file, line context, or which resource/data source it came from.

Warning: Deprecated Resource

"ids": [DEPRECATED] Use full_names instead. The ids attribute will be removed
in the future. See the CHANGELOG to learn more:
https://github.com/hashicorp/terraform-provider-tfe/blob/v0.18.0/CHANGELOG.md   

Steps to Reproduce

1. Make sure you have an account and user API token for Terraform Cloud. Add the user token to your ~/.terraformrc file.
2. Create a local build of the TFE provider by following the instructions in the README. I used a local filesystem mirror.
3. Save the configuration provided above, substituting your own information as needed.
4. terraform init
5. terraform plan

References

#561 seems similar but not exactly the same.

[chrisarcand]

I think what's happened here is that the new SDK is making a very fair assumption that schema.Resource's DeprecationMessage refers to the resource as a whole and not to a specific field within that resource, and has happened to have changed the output. Why the line trace was removed, I'm uncertain, and perhaps @paddycarver can shed some light there?

I think the answer is "this is not what DeprecationMessage is for" and TFE provider so happened to have used it because of the known limitations of warning when a deprecated field is accessed, which you pointed out well in hashicorp/terraform-provider-tfe#253

Sadly though, I believe that's not the specific provider's concern and the right thing to do here is remove that and accept schema.Schema.Deprecated's limitations, even if it means not having the deprecation message in the plan output for now.

[lafentres]

@chrisarcand Yeah totally. I don't take issue with the addition of Warning: Deprecated Resource at all. I think that's actually an improvement on the previous behavior for the intended use case. I like it!

The only thing that is actually a problem for me is that the pointer to the configuration file and line context/number are gone. Without that, there's no way to tell what resource a warning is coming from.

[paddycarver]

I agree with @chrisarcand's interpretation of this issue, I think. This isn't really the intended usage of DeprecationMessage, and will (I think) warn even when users aren't using the ids attribute, which isn't ideal.

But it shouldn't have lost its file/line context, and I think that is related to some diagnostics work we did in v2 of the SDK. There are a number of issues with diagnostics not retaining file/line numbers properly, and we just need to do a sweep to make sure they're getting bubbled up correctly everywhere.

[paddycarver]

I've dug into this a bit more, and as far as I can tell, this is an upstream terraform issue. Have you seen it work with v1 and not with v2 using the same version of Terraform?

Diagnostics are how we return error and warning information like this to Terraform, and they're getting populated (as you can see by the message appearing at all). They're not populating the AttributePath, which is how we indicate which part of a resource is causing the error... but that's right, too, because the DeprecationMessage helper is meant to point to the whole resource.

The fact that the warning is showing up, therefore, leads me to believe we're doing everything necessary on the SDK side of the gRPC barrier. Terraform is where those errors get associated with config filenames and lines, and as far as I can tell, is where it's going awry. But poking around a bit, I'm seeing the code I would expect in the call to validate. Does this work with terraform validate and not with plan? What version of Terraform is this? I notice there was some refactoring there, recently-ish.

[lafentres]

I agree with @chrisarcand's interpretation of this issue, I think. This isn't really the intended usage of DeprecationMessage, and will (I think) warn even when users aren't using the ids attribute, which isn't ideal.

Absolutely! It definitely does warn even when users aren't using the ids attribute and that definitely isn't ideal. It seemed like an okay trade-off here, since there wasn't really any other way for me to provide a deprecation message for this (that I'm aware of, the deprecation field on the schema attribute doesn't work yet). The loss of the line number and file was the only issue here.

I've dug into this a bit more, and as far as I can tell, this is an upstream terraform issue. Have you seen it work with v1 and not with v2 using the same version of Terraform?

Yeah, when I first noticed this I did some testing to see if it was something I changed that broke things. I tested it with v1 (version 0.23.0 of the TFE provider) & Terraform 0.13.5 and it worked as expected. Then I tested it with v2 (this version of the TFE provider hasn't been released yet so I was working with a local build from master), still using Terraform 0.13.5 and saw the missing line number/file context.

[paddycarver]

Incredibly bizarre! I'll poke at this some more and see if I can't track down what the heck happened here, because I'm not going to lie, I'm stumped.

[paddycarver]

This is likely related to #696.

[paddycarver]

I think #696 should have resolved this. If anyone is still running into it when using SDK 2.4.4 or higher, please let us know. :)

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.