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

feat(codestarnotifications): add createdBy property for notification rule #30780

Merged
merged 4 commits into from
Jul 9, 2024

Conversation

yuppe-kyupeen
Copy link
Contributor

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


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@github-actions github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Jul 8, 2024
@aws-cdk-automation aws-cdk-automation requested a review from a team July 8, 2024 07:03
@github-actions github-actions bot added the p2 label Jul 8, 2024
@yuppe-kyupeen
Copy link
Contributor Author

Exemption Request: No changes were made to the README.md as this is a minor modification.

@aws-cdk-automation aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Jul 8, 2024
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a 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.

@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jul 8, 2024
@badmintoncryer
Copy link
Contributor

@yuppe-kyupeen Thank you for your PR! My only advice is to add a README description because this is not a minor modification😁

@yuppe-kyupeen yuppe-kyupeen force-pushed the add-codestarnotifications-props branch from e8a7778 to eb96eef Compare July 8, 2024 12:04
@aws-cdk-automation aws-cdk-automation dismissed their stale review July 8, 2024 12:06

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

@yuppe-kyupeen
Copy link
Contributor Author

@badmintoncryer
Thank you for your advice!
I have made the changes! If you have any other suggestions, I would appreciate your feedback.

Copy link
Contributor

@go-to-k go-to-k left a 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.

Comment on lines 15 to 28
### 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.

Copy link
Contributor

@go-to-k go-to-k Jul 9, 2024

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.

Suggested change
### 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
...
...
```

Copy link
Contributor

@go-to-k go-to-k Jul 9, 2024

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],
});

https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-codestarnotifications/README.md#examples

Copy link
Contributor Author

@yuppe-kyupeen yuppe-kyupeen Jul 9, 2024

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?😅

Copy link
Contributor

Choose a reason for hiding this comment

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

@yuppe-kyupeen

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!

Copy link
Contributor Author

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🙏

@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jul 9, 2024
@yuppe-kyupeen yuppe-kyupeen force-pushed the add-codestarnotifications-props branch from eb96eef to da7ab12 Compare July 9, 2024 07:55
@yuppe-kyupeen yuppe-kyupeen changed the title feat(codestarnotifications): add createdBy property feat(codestarnotifications): add createdBy property for notification rule Jul 9, 2024
Copy link
Contributor

@go-to-k go-to-k left a 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.

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jul 9, 2024
Copy link
Contributor

mergify bot commented Jul 9, 2024

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-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jul 9, 2024
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 40d8c28
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit a68b418 into aws:main Jul 9, 2024
12 checks passed
Copy link
Contributor

mergify bot commented Jul 9, 2024

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).

@yuppe-kyupeen yuppe-kyupeen deleted the add-codestarnotifications-props branch July 9, 2024 21:39
GavinZZ pushed a commit that referenced this pull request Jul 10, 2024
…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*
@aws-cdk-automation
Copy link
Collaborator

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.

@aws aws locked as resolved and limited conversation to collaborators Jul 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants