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

fix: Include backend in dependent bodies to avoid false negatives #251

Merged
merged 1 commit into from
Sep 4, 2023

Conversation

radeksimko
Copy link
Member

@radeksimko radeksimko commented Sep 4, 2023

Context

A bug was discovered as part of testing hashicorp/terraform-ls#1368 - see below.

CleanShot 2023-09-01 at 17 03 14


The assumption I made in the beginning of investigating this bug - which is likely the same assumption made while the bug was introduced in the first place - that the backend attribute is part of a core schema which we'd end up keeping after merge, just like we keep depends_on and similar ones.

However, the terraform_remote_state data source is a bit different in that the "merge hierarchy" is much flatter, where the "non-backend" body is on the same level as the "backend-specific" body, so no merging can ever happen between the two:

// data block
DependentBody: {
     "{\"labels\":[{\"index\":0,\"value\":\"terraform_remote_state\"}]}": &schema.BodySchema{
                                Attributes: {
                                    "backend": &schema.AttributeSchema{
                                        IsRequired:   true,
                                        Constraint:   schema.OneOf{
                                            schema.LiteralValue{
                                                Value:        cty.StringVal("azurerm"),
                                                IsDeprecated: false,
                                                Description:  lang.MarkupContent{Value:"Azure Blob Storage", Kind:0x2},
                                            },
// ...
                                            schema.LiteralValue{
                                                Value:        cty.StringVal("s3"),
                                                IsDeprecated: false,
                                                Description:  lang.MarkupContent{Value:"Amazon S3 (with locking via DynamoDB)", Kind:0x2},
                                            },
                                        },
                                        IsDepKey:               true,
                                        SemanticTokenModifiers: {"hcl-dependent"},
                                    },
     "{\"labels\":[{\"index\":0,\"value\":\"terraform_remote_state\"}],\"attrs\":[{\"name\":\"backend\",\"expr\":{\"static\":\"s3\"}}]}": &schema.BodySchema{
                    Blocks:     {},
                    Attributes: {
// ... (no backend attribute here)

Implementation Notes

I initially thought we could just set the backend Constraint to the given schema.LiteralValue{} for each backend. While this would address the validation problem, it would also make it much more difficult to solve hashicorp/terraform-ls#1284 as we wouldn't have access to all the other backends and their details at completion time, when the merging has already taken place (because backend has matching value).

@radeksimko radeksimko force-pushed the b-fix-remote-state-backend-validation branch from f1de9cc to 0a39159 Compare September 4, 2023 12:28
@radeksimko radeksimko marked this pull request as ready for review September 4, 2023 12:35
@radeksimko radeksimko requested a review from a team as a code owner September 4, 2023 12:35
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 description

@radeksimko radeksimko merged commit 90a3970 into main Sep 4, 2023
4 checks passed
@radeksimko radeksimko deleted the b-fix-remote-state-backend-validation branch September 4, 2023 12:54
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