feat(codestarnotifications): add createdBy property for notification rule#30780
Conversation
|
Exemption Request: No changes were made to the README.md as this is a minor modification. |
aws-cdk-automation
left a comment
There was a problem hiding this comment.
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.
|
@yuppe-kyupeen Thank you for your PR! My only advice is to add a README description because this is not a minor modification😁 |
e8a7778 to
eb96eef
Compare
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
|
@badmintoncryer |
go-to-k
left a comment
There was a problem hiding this comment.
Thanks for your contribution!
It might be good to change the PR title to feat(codestarnotifications): add `createdBy` property for notification rule.
| ### notificationRuleName | ||
| You can set a name for the notification rule. Notification rule names must be unique in your AWS account. | ||
|
|
||
| ### enabled | ||
| You can set the status of the notification rule.If the enabled is set to DISABLED, notifications aren't sent for the notification rule.The default is true. | ||
|
|
||
| ### detailType | ||
| You can set the level of detail to include in the notifications for this resource. | ||
| BASIC will include only the contents of the event as it would appear in AWS CloudWatch. | ||
| FULL will include any supplemental information provided by AWS CodeStar Notifications and/or the service for the resource for which the notification is created. | ||
|
|
||
| ### createdBy | ||
| You can set a name or email alias of the person who created the notification rule.If not specified, it means that the creator's alias is not provided. | ||
|
|
There was a problem hiding this comment.
Could you please add sample code for the construct?
Also, I think that these notificationRuleName, enabled, detailType and createdBy blocks are the same as the JSDoc for the properties in the construct, so they could be erased. The sample code could cover them.
| ### notificationRuleName | |
| You can set a name for the notification rule. Notification rule names must be unique in your AWS account. | |
| ### enabled | |
| You can set the status of the notification rule.If the enabled is set to DISABLED, notifications aren't sent for the notification rule.The default is true. | |
| ### detailType | |
| You can set the level of detail to include in the notifications for this resource. | |
| BASIC will include only the contents of the event as it would appear in AWS CloudWatch. | |
| FULL will include any supplemental information provided by AWS CodeStar Notifications and/or the service for the resource for which the notification is created. | |
| ### createdBy | |
| You can set a name or email alias of the person who created the notification rule.If not specified, it means that the creator's alias is not provided. | |
| ```ts | |
| ... | |
| ... | |
| ``` |
There was a problem hiding this comment.
Sorry, it seems that the README already has the example, so it might be good to add the properties here.
const rule = new notifications.NotificationRule(this, 'NotificationRule', {
// add the properties here: notificationRuleName, enabled, detailType, and createdBy
source: project,
events: [
'codebuild-project-build-state-succeeded',
'codebuild-project-build-state-failed',
],
targets: [topic],
});There was a problem hiding this comment.
@go-to-k
Thank you for your review!
I've made the changes!
By the way, even though I haven't specifically changed the integ-test, the CI process "request-cli-integ-test" is failing. Do you know why that might be?😅
There was a problem hiding this comment.
Thanks for the change!
By the way, even though I haven't specifically changed the integ-test, the CI process "request-cli-integ-test" is failing. Do you know why that might be?😅
Pulling from the main branch or pushing a new commit and running CI again often fixes it!
eb96eef to
da7ab12
Compare
createdBy propertycreatedBy property for notification rule
go-to-k
left a comment
There was a problem hiding this comment.
Thank you @yuppe-kyupeen ! Approved.
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
…n rule (#30780) ### Issue # (if applicable) ### Reason for this change The `createdBy` property existed in the L1 construct but was not present in the L2 construct ### Description of changes - Add the `createdBy` property for `NotificationRule`, which was missing in the L2 construct. ### Description of how you validated changes I Added a unit test for codestarnotifications and integration tests for pipeline and codestarnotifications ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
|
Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one. |
Issue # (if applicable)
Reason for this change
The
createdByproperty existed in the L1 construct but was not present in the L2 constructDescription of changes
createdByproperty forNotificationRule, which was missing in the L2 construct.Description of how you validated changes
I Added a unit test for codestarnotifications and integration tests for pipeline and codestarnotifications
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license