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

firehose addon module updates #15439

Merged
merged 3 commits into from
Dec 8, 2023
Merged

Conversation

edwardsb
Copy link
Contributor

@edwardsb edwardsb commented Dec 4, 2023

refactor module to use latest terraform provider and update necessary resource declarations, audit stream in no longer optional, updated results, status, and audit prefix formats to be out-of-the-box compatible with athena date-time partition formats

Checklist for submitter

If some of the following don't apply, delete the relevant line.

  • Changes file added for user-visible changes in changes/ or orbit/changes/.
    See Changes files for more information.
  • Manual QA for all new/changed functionality

Sorry, something went wrong.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
… resource declarations, audit stream in no longer optional, updated results, status, and audit prefix formats to be out-of-the-box compatible with athena date-time partition formats
@edwardsb edwardsb requested a review from a team as a code owner December 4, 2023 21:57
@edwardsb
Copy link
Contributor Author

edwardsb commented Dec 4, 2023

@rfairburn my intention here is to "major" version bump this addon since I made some semi-breaking changes:

  1. audit stream is no longer optional
  2. default results & status prefixes have changed

Copy link
Contributor

@rfairburn rfairburn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If my assumptions are correct, this all LGTM.

@@ -27,16 +27,82 @@ variable "fleet_iam_role_arn" {
}

variable "results_prefix" {
default = "results/"
default = "results/year=!{timestamp:yyyy}/month=!{timestamp:MM}/day=!{timestamp:dd}/"
Copy link
Contributor

@rfairburn rfairburn Dec 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for the partitioning strategy we talked about previously on slack right? We might want to document why this looks like this so that when it gets overridden by someone passing in a var they don't lose the benefit because they didn't know.

rfairburn
rfairburn previously approved these changes Dec 4, 2023
@edwardsb edwardsb merged commit eb7f838 into main Dec 8, 2023
@edwardsb edwardsb deleted the edwardsb-firehose-addon-refactor branch December 8, 2023 00:24
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.

None yet

2 participants