-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Adding support to get details for TargetPrefix #767
Conversation
! Note that this needs the terratest PR (gruntwork-io/terratest#767) to be merged in to work & be tested.
modules/aws/s3.go
Outdated
} | ||
|
||
return aws.StringValue(res.LoggingEnabled.TargetBucket), nil | ||
return aws.StringValue(res.LoggingEnabled.TargetBucket), aws.StringValue(res.LoggingEnabled.TargetPrefix), nil |
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.
To maintain backwards compatibility, should there be a separate method to return just the target prefix? Or possibly the bucket + prefix?
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.
+1 to introduce a separate method that just retrieves the target prefix.
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.
That's a really good point! Thanks a lot both 🙂
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.
So in terms of backwards compatibility - did you also mean to keep the naming of the method as it was originally? It was quite a generic name, so there's now 2 functions: GetS3BucketLoggingTargetBucket
& GetS3BucketLoggingTargetPrefix
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.
Yea, I think preserving the old name makes sense too. It's not an ideal name, but probably not bad enough it merits a backwards incompatible 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.
Ok, will change that too - thanks, Jim!
Took me a while in the midst of support to come back here (I thought it would require a lot more changes, but turns out it was this quick). Another look would be very appreciated! |
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.
LGTM!
Thanks Jim! |
* adding target-prefix ro access bucket logging * Updating test & example ! Note that this needs the terratest PR (gruntwork-io/terratest#767) to be merged in to work & be tested. * Updating Terratest dependency * testing for target prefix * Updating docs * Renaming folder * Updating to Debugf * Adding default value * WIP - parsing for TFstatelogs * Updating logic & docs * Adding a new test for default TargetPrefix in remote backend config
* Fix dead link in multiple aws accounts docs (gruntwork-io#1563) * Fix dead link in multiple aws accounts docs The link to AWS docs is now 404. The corrected link seems to most closely resemble the intended target. Other options to consider: https://aws.amazon.com/organizations/getting-started/best-practices/ https://docs.aws.amazon.com/controltower/latest/userguide/aws-multi-account-landing-zone.html * Link to AWS best practices for multi account docs * Whitespace removal (gruntwork-io#1573) * Fix empty outputs (gruntwork-io#1568) If stack run finished without errors, `summarizePlanAllErrors()` receives empty buffer and outputs empty line. This change ensures that only non-empty outputs are getting logged. Related: gruntwork-io#1541 * doc: contributing: fix broken link to circleci (gruntwork-io#1580) * Bump AWS SDK to version v1.37.7 to support AWS SSO (gruntwork-io#1537) * Add TargetPrefix as config input to access bucket logging (gruntwork-io#1507) * adding target-prefix ro access bucket logging * Updating test & example ! Note that this needs the terratest PR (gruntwork-io/terratest#767) to be merged in to work & be tested. * Updating Terratest dependency * testing for target prefix * Updating docs * Renaming folder * Updating to Debugf * Adding default value * WIP - parsing for TFstatelogs * Updating logic & docs * Adding a new test for default TargetPrefix in remote backend config * Introduce validate-inputs, which can be used to check for variable alignment (gruntwork-io#1572) * Introduce terragrunt-input-info, which can be used to check for variable alignment * Apply suggestions from code review Co-authored-by: Zack Proser <[email protected]> * Tidy go modules * Renamed input-info to validate-inputs * Switch missing required vars to errors * Handle -var and -var-file args * Update cli/validate_inputs.go Co-authored-by: Yevgeniy Brikman <[email protected]> * Make sure to check for dynamically passed in CLI args * Fix build * Handle automatically loaded var files * Remove plan args check * Clarify difference between getTerraformInputNamesFromVarFiles and getTerraformInputNamesFromCLIArgs * Address PR nit to move example in docs Co-authored-by: Zack Proser <[email protected]> Co-authored-by: Yevgeniy Brikman <[email protected]> * Use go1.16 to build arm64 binaries (gruntwork-io#1585) * Bump creack/pty to 1.1.11 (gruntwork-io#1582) Co-authored-by: Andy Bohne <[email protected]> * Add ability to specify working directory of hooks (gruntwork-io#1588) * Add ability to specify working directory of hooks * Fix build * Support dynamodb_endpoint attribute of S3 backend (gruntwork-io#1586) * Clarify non-interactive will not include external dependencies (gruntwork-io#1593) * add getTerragruntSource helper function (gruntwork-io#1575) * add getTerragruntSource helper function * update docs * update docs and tests for get_terragrunt_source_cli_flag() function * add use cases for get_terragrunt_source_cli_flag * Recursively extract forcedgetters until there are none (gruntwork-io#1594) * Remove all usage of get-plugins=false which is removed in 0.15.0 (gruntwork-io#1618) * Fix validate-inputs to support null defaults (gruntwork-io#1613) * Clarify context of find_in_parent_folders (gruntwork-io#1623) Co-authored-by: Paul <[email protected]> Co-authored-by: Yoriyasu Yano <[email protected]> Co-authored-by: amnk <[email protected]> Co-authored-by: Marco Molteni <[email protected]> Co-authored-by: David Wooldridge <[email protected]> Co-authored-by: Ina Stoyanova <[email protected]> Co-authored-by: Zack Proser <[email protected]> Co-authored-by: Yevgeniy Brikman <[email protected]> Co-authored-by: Andy Bohne <[email protected]> Co-authored-by: Andy Bohne <[email protected]> Co-authored-by: Alexey Remizov <[email protected]> Co-authored-by: Syed Hussain <[email protected]> Co-authored-by: David Alger <[email protected]>
This PR adds the functionality needed in this code changes: gruntwork-io/terragrunt#1507
With this PR we can:
TargetPrefix
specified for a state bucket with enabled access loggingIssue linked: gruntwork-io/terragrunt#1437