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

decoder: Ensure partially unknown dependent body is handled #339

Merged
merged 2 commits into from
Nov 3, 2023

Conversation

radeksimko
Copy link
Member

@radeksimko radeksimko commented Nov 2, 2023

As demonstrated by the attached test, we can have a situation of "nested dependent modules" which was previously handled incorrectly. This is best explained on an example.

variable "terraform_rs" {
  default = {
    backend              = "azurerm"
    workspace            = "***"
    storage_account_name = "***"
    container_name       = "***"
    key                  = "***"
    access_key           = "***"
  }
}

data "terraform_remote_state" "this" {
  backend   = var.terraform_rs.backend
  workspace = var.terraform_rs.workspace
  config = {
    storage_account_name = var.terraform_rs.storage_account_name
    container_name       = var.terraform_rs.container_name
    key                  = var.terraform_rs.key
    access_key           = var.terraform_rs.access_key
  }
}

Above, we have multiple dependent body schemas for the data block:

  • The first one is dependent on 1st label matching terraform_remote_state - that brings us backend, workspace and other attributes generally relevant for that particular data source
  • The second one is then dependent on the value of backend (i.e. attribute which was brought in as dependent from the earlier merging).

In the example above we would succeed in the first lookup, but fail in the second one - because we don't evaluate the expression in this context and cannot tell what var.terraform_rs.backend actually is. This is basically equivalent to any other invalid value, such as backend = "foobar" but the example above is a little more realistic. Now we report a more granular state reflecting the reality more accurately.

This is perhaps not ideal in the sense that although we suppress irrelevant diagnostics, we also suppress some helpful ones, e.g. invalid backend

data "terraform_remote_state" "this" {
  backend   = "foobar"
  workspace = "noot"
}

Also we could run the validation in the top example with interpolated var.terraform_rs.backend, i.e. validate config in the context of backend = "azurerm" but interpolation is whole another problem we haven't really tackled yet.

We can treat this as an opportunity for future enhancements while reducing false negatives in the meantime.

UX Before

Screenshot 2023-11-02 at 14 28 16

UX After

Screenshot 2023-11-02 at 14 30 28

@radeksimko radeksimko force-pushed the b-partially-unknown-dep-body branch from 26ba301 to ab94513 Compare November 2, 2023 14:16
@radeksimko radeksimko marked this pull request as ready for review November 2, 2023 15:46
@radeksimko radeksimko requested a review from a team as a code owner November 2, 2023 15:46
Copy link
Member

@dbanck dbanck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Appreciate the detailed examples

@radeksimko
Copy link
Member Author

I created follow-up issues as mentioned:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants