-
Notifications
You must be signed in to change notification settings - Fork 0
feat(outputs)!: breaking change, add sensitive true to outputs + add tests #17
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simple + straightforward!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple questions, but I like that we're circling back to this module to add tests since we do use it a good bit!
output "all" { | ||
value = local.secrets | ||
description = "The final secrets pulled from various sources." | ||
sensitive = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You call this out as a breaking change in the PR title, but I'm not sure it is. Mind sharing why you're considering this a breaking change? It changes the output of this module, but I would just call that a feature enhancement, not a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am expecting that adding sensitive = true
requires downstream child or root modules then also mark the output as sensitive. My practical concern is that users would need to make a code change on their end in order to get terraform plan/applies to run.
For example, https://developer.hashicorp.com/terraform/tutorials/configuration-language/sensitive-variables#reference-sensitive-variables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this is interesting. I wasn't aware of this behaviour, but we have to treat this as a breaking change indeed.
@westonplatter could you please rename a PR title to reflect this: feat(outputs)!: add sensitive true to outputs + add tests)
, so release-please correctly generates the release.
Also, we should ensure that this is described in the release notes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@westonplatter good thinking in terms of this breaking downstream outputs, you're obviously right that this is a breaking change 💯 👍
## what - Update with the latest template. ## why - Unblock existing PRs - checks are stuck. ## references - #17
what
outputs.tf
to include sensitive true.why
Impacts
For any downstream root modules using this child module, they'd need to also used
sensitive = true
when accessing the values. In the examples/complete/outputs.tf file, we already do this, but I think this would be a breaking change?