Added functionality to store github events in s3 bucket #85
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #85 +/- ##
============================================
+ Coverage 83.48% 83.57% +0.09%
Complexity 202 202
============================================
Files 55 56 +1
Lines 1229 1236 +7
Branches 51 52 +1
============================================
+ Hits 1026 1033 +7
Misses 181 181
Partials 22 22 ☔ View full report in Codecov by Sentry. |
infrastructure/lib/stacks/s3.ts
Outdated
| super(scope, id); | ||
|
|
||
| this.githubEventsBucket = new Bucket(this, 'OpenSearchMetricsGithubEvents', { | ||
| bucketName: "opensearch-project-github-events", |
There was a problem hiding this comment.
Do we want to name the bucket? Usually it is not recommended to name the resources created via CDK. Let the CDK assign the random naming to avoid future failures.
There was a problem hiding this comment.
Just curious, why is it not recommended? We are indexing to this bucket in the automation app here: https://github.com/opensearch-project/automation-app/blob/main/src/call/github-events-to-s3.ts#L29
There was a problem hiding this comment.
s3 bucket names are unique globally (not just within the account but all accounts in AWS). When you name the resources, you cannot deploy multiple stacks using same CDK as it will throw an error with resource already exists. All the resources in CDK have a resource identifier or logical name but adding a resource name might not be a good idea. You can deploy the stack and use the name created by the CDK in the automation app url mentioned above.
There was a problem hiding this comment.
Sounds good, removed the bucket name and changed the logical ID to be more specific
There was a problem hiding this comment.
@bshien you can add the bucket name to the secret and use the secret here https://github.com/opensearch-project/automation-app/blob/main/src/call/github-events-to-s3.ts#L29.
There was a problem hiding this comment.
Other option we can go is allow the bucketName to passed as a stack props, then its upto the user to pass the bucket name.
There was a problem hiding this comment.
Check out this PR here: opensearch-project/automation-app#26
There was a problem hiding this comment.
Just added one comment here https://github.com/opensearch-project/automation-app/pull/26/files#r1793853695, in that case just create the bucket and update the secret. Thanks
bshien
force-pushed
the
s3-event-sink-3
branch
2 times, most recently
from
bade547
to
46df9d5
Compare
DEVELOPER_GUIDE.md
Outdated
| @@ -25,6 +25,7 @@ So you want to contribute code to this project? Excellent! We're glad you're her | |||
| - `cdk deploy OpenSearchMetrics-GitHubAutomationApp-Secret`: Creates the GitHub app secret which will be used during the GitHub app runtime. | |||
| - `cdk deploy OpenSearchMetrics-GitHubWorkflowMonitor-Alarms`: Creates the Alarms to Monitor the Critical GitHub CI workflows by the GitHub Automation App. | |||
| - `cdk deploy OpenSearchMetrics-GitHubAutomationApp`: Create the resources which launches the [GitHub Automation App](https://github.com/opensearch-project/automation-app). Listens to GitHub events and index the data to Metrics cluster. | |||
| - `cdk deploy OpenSearchMetrics-S3`: Create the S3 Bucket that will store GitHub Events. | |||
There was a problem hiding this comment.
Can you please change this to OpenSearchMetrics-GitHubAutomationAppEvents-S3
There was a problem hiding this comment.
I'm thinking we can put various s3 buckets in this stack in the future, so the name should be kept general. I'll change the description to describe this
There was a problem hiding this comment.
Yes since you added under infrastructure/lib/stacks/s3.ts it will create new stack for new s3 bucket, so the stack name is OpenSearchMetrics-GitHubAutomationAppEvents-S3. With this we have have a proper naming convention for each s3 bucket stack.
So tomorrow if you need a new s3 bucket it will be a new stack altogether.
There was a problem hiding this comment.
Got it, I updated the PR to make these changes
bshien
force-pushed
the
s3-event-sink-3
branch
from
46df9d5
to
ed102d6
Compare
bshien
force-pushed
the
s3-event-sink-3
branch
from
ed102d6
to
2f42089
Compare
Signed-off-by: Brandon Shien <bshien@amazon.com>
bshien
force-pushed
the
s3-event-sink-3
branch
from
2f42089
to
f5a4c7f
Compare
Description
Coming from #76
Issues Resolved
Part of #57
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.