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(stack): introduce configsubscription variable #140

Merged
merged 1 commit into from
Apr 26, 2024

Conversation

jta
Copy link
Contributor

@jta jta commented Apr 26, 2024

This commit addresses a limitation of our current module API whereby passing a bucket reference would raise an error, e.g.:

module "forwarder" {
  ...
  config_delivery_bucket_name = aws_s3_bucket.this.id
}

would trigger:

│ Error: Invalid count argument
│
│   on .terraform/modules/forwarder/modules/stack/configsubscription.tf line 2, in module "configsubscription":
│    2:   count  = var.config_delivery_bucket_name != null ? 1 : 0
│
│ The "count" value depends on resource attributes that cannot be determined until apply, so Terraform cannot predict how many instances will be created. To work around this, use the -target argument to
│ first apply only the resources that the count depends on.

To work around this, we can instead gate the condition on the presence of an object, which is the pattern we follow for all other nested modules:

module "forwarder" {
  ...
  configsubscription = {
    delivery_bucket_name = aws_s3_bucket.this.id
  }
}

In this case, we no longer need to worry about whether delivery_bucket_name is known, since our condition will verify the presence of configsubscription instead.

@jta jta requested a review from a team as a code owner April 26, 2024 17:36
@jta jta requested a review from obs-gh-abhishekrao April 26, 2024 17:36
@obs-gh-abhishekrao
Copy link
Contributor

@jta I see we pick up tests on this repo. Can you add one for these changes? We should have caught the error this fixes through one of those
https://github.com/observeinc/terraform-aws-collection/blob/main/examples/simple/tests/simple.tftest.hcl

This commit addresses a limitation of our current module API whereby
passing a bucket reference would raise an error, e.g.:

```
module "forwarder" {
  ...
  config_delivery_bucket_name = aws_s3_bucket.this.id
}
```

would trigger:
```
│ Error: Invalid count argument
│
│   on .terraform/modules/forwarder/modules/stack/configsubscription.tf line 2, in module "configsubscription":
│    2:   count  = var.config_delivery_bucket_name != null ? 1 : 0
│
│ The "count" value depends on resource attributes that cannot be determined until apply, so Terraform cannot predict how many instances will be created. To work around this, use the -target argument to
│ first apply only the resources that the count depends on.
```

To work around this, we can instead gate the condition on the
presence of an object, which is the pattern we follow for all other
nested modules:

```
module "forwarder" {
  ...
  configsubscription = {
    delivery_bucket_name = aws_s3_bucket.this.id
  }
}
```

In this case, we no longer need to worry about whether
`delivery_bucket_name` is known, since our condition will verify the
presence of `configsubscription` instead.
@jta jta force-pushed the joao/configsubscriptionvar branch from 59c92fe to 835db78 Compare April 26, 2024 18:53
@jta
Copy link
Contributor Author

jta commented Apr 26, 2024

I can extend the tests, it just doesn't really cover the bug because in .tftest.hcl we create a bucket in a separate run block. Each block has its own terraform state, so we don't actually get an end-to-end apply upfront.

@obs-gh-abhishekrao
Copy link
Contributor

I can extend the tests, it just doesn't really cover the bug because in .tftest.hcl we create a bucket in a separate run block. Each block has its own terraform state, so we don't actually get an end-to-end apply upfront.

Got it. In any case we might need to add tests for the filedrop approach if we don't already have one so I'll leave it to you whether it should happen in this PR or a different one.

@jta jta merged commit 4906a0f into main Apr 26, 2024
32 checks passed
@jta jta deleted the joao/configsubscriptionvar branch April 26, 2024 20:06
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