-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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(codestarnotifications): add createdBy
property for notification rule
#30780
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. |
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.
@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 |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
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.
@go-to-k
It's resolved! Thank you🙏
eb96eef
to
da7ab12
Compare
createdBy
propertycreatedBy
property for notification rule
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 @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
createdBy
property existed in the L1 construct but was not present in the L2 constructDescription of changes
createdBy
property 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