feat: add support for optional S3 secondary artifact #78
Conversation
jhosteny
requested review from
3h4x and
joe-niland
and removed request for
a team
jhosteny
force-pushed
the
feat/secondary-artifact
branch
from
150b0fd to
4cea1c8
Compare
|
/test all |
|
/rebuild-readme |
|
/test all |
main.tf
Outdated
| type = "S3" | ||
| location = var.secondary_artifact_location | ||
| artifact_identifier = var.secondary_artifact_identifier | ||
| encryption_disabled = true |
There was a problem hiding this comment.
Can you add an additional variable for secondary_artifact_encryption_enabled, set the default to false, and use that var here? All Cloud Posse modules are trying to pass BridgeCrew security scanning and I'd bet this might fail in the future if the default / hardcoded value is false.
There was a problem hiding this comment.
@Gowiem just to clarify, do you want secondary_artifact_encryption_enabled, with it set to false, or secondary_artifact_encryption_disabled, with it set to false? I'm guessing you actually meant the latter?
There was a problem hiding this comment.
I went ahead with that assumption that the variable name should be secondary_artifact_encryption_disabled, and set to false. I can fix if this is not what you intended.
There was a problem hiding this comment.
Ah sorry @jhosteny I did mean secondary_artifact_encryption_enabled. We have _enabled as a convention for any new bool vars. Please update if you don't mind.
|
/test all |
|
/test all |
|
@Gowiem this has been updated per your last comment. However, |
|
@jhosteny ah frustrating. Let's try building via ChatOps and see if it will pass after that... |
|
/rebuild-readme |
|
/test readme |
|
@Gowiem does this look okay now? |
|
This pull request is now in conflict. Could you fix it @jhosteny? 🙏 |
|
/test all |
|
/test all |
|
This pull request is now in conflict. Could you fix it @jhosteny? 🙏 |
|
/test all |
Fixes #77.
what
why
references
Closes #77