-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat(pipelines): Add logging options to codeBuildDefaults property #24016
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.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.
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.
Thank you for your contribution! Really great work here, I just have a few comments inline.
logging: { | ||
cloudWatch: { | ||
logGroup: logGroup, | ||
prefix: 'prefix', | ||
enabled: true, | ||
}, | ||
s3: { | ||
encrypted: true, | ||
bucket: bucket, | ||
prefix: 's3prefix', | ||
enabled: 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.
I'm wondering if we can make this a little less manual for the user. For instance, if they're adding cloudwatch
logging, I'd assume that they want enabled: true
. Can we abstract some of this away? Maybe with a class similar to Schedule
in aws-events
that uses an enum like class. What do you think?
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 agree about providing human-friendly interfaces in high-level constructs. However, it would let users lose control if we overpack the functions. Users are able to set options separately for CloudWatch & S3 logging in aws-codebuild
API, but not for Schedule
in aws-events
API.
I prefer to keep the current solution. But in the future, if any contributor requests a similar feature, we can design the interface.
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 @TheRealAmazonKendra's suggestion. Users will still be able to maintain full control. We could potentially do a separate property: eg, cloudwatchLogging
, s3Logging
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.
Before abstracting the logging setting, I'd like to confirm the expected result when the user doesn't assign the object to CloudWatch logging. For now, CloudWatch logging is still enabled and saves the log to the default pattern.
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.
Let's leave the behavior as is if the user doesn't provide any input. I suppose we could have something like Logging.NONE()
for logs to be disabled altogether, but we should leave the default alone so we don't unintentionally break customers code that might rely on it.
* | ||
* @default - no log configuration is set | ||
*/ | ||
readonly logging?: codebuild.LoggingOptions; |
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.
My comment above may mean refactoring LoggingOptions
instead of building a new interface for this specifically. In fact, that's probably the best path forward. We could improve this experience in general.
export function mergeLoggings(a: codebuild.LoggingOptions, b?: codebuild.LoggingOptions): codebuild.LoggingOptions | undefined; | ||
export function mergeLoggings(a: codebuild.LoggingOptions | undefined, b: codebuild.LoggingOptions): codebuild.LoggingOptions | undefined; | ||
export function mergeLoggings(a?: codebuild.LoggingOptions, b?: codebuild.LoggingOptions): codebuild.LoggingOptions | undefined; | ||
export function mergeLoggings(a?: codebuild.LoggingOptions, b?: codebuild.LoggingOptions) { |
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.
Add spacing between these for my delicate eyeballs, please.
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.
Granted, with my suggestions above, these might not be needed.
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.
Is there any suggestion for a coding style?
It seems that other overloading functions are followed like this.
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 know, I didn't even notice that it was an overloaded function because I reacted too quickly to the spacing issue. This is appropriate and my eyeballs will deal, lol.
packages/@aws-cdk/pipelines/test/integ.pipeline-with-logging.ts
Outdated
Show resolved
Hide resolved
Pull request has been modified.
logging: { | ||
cloudWatch: { | ||
logGroup: logGroup, | ||
prefix: 'prefix', | ||
enabled: true, | ||
}, | ||
s3: { | ||
encrypted: true, | ||
bucket: bucket, | ||
prefix: 's3prefix', | ||
enabled: 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.
👍 to @TheRealAmazonKendra's suggestion. Users will still be able to maintain full control. We could potentially do a separate property: eg, cloudwatchLogging
, s3Logging
Pull request has been modified.
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.
Putting this back into changes requested to reflect the status.
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
1 similar comment
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
This PR has been in the BUILD FAILING state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week. |
@yo-ga, circling back on this, what information do you need to move forward with the refactor? Read through the context but summarize for me so I can catch up. |
@MrArnoldPalmer sorry for missing some days. |
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
1 similar comment
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
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.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request
. Additionally, if clarification is needed add Clarification Request
to a comment.
✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
This feature has been already implemented from #25266. I would close this PR. |
Add logging options to pipeline.codeBuildDefaults property.
Closes #22045 #23676.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license