feat(events): add filter rules for prefixEqualsIgnoreCase, suffixEqualsIgnoreCase, wildcard, and anythingBut* matches #32063
Conversation
github-actions
bot
added
effort/medium
|
The build fails, which seems to be due to failing tests for It's not clear from the contributing guidelines what/how to fix. |
|
@jaw111 Thank you for your contribution! It appears that aws-events integ test has failed, which may indicate that this modification introduces a breaking change. @aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3594926594/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-events/test/integ.rule.js
@aws-cdk-testing/framework-integ: Error: Some tests failed!Could you please clarify the reason for failure? |
|
Oh, it seems the integration test you updated this time has failed. Could you please update the snapshot for |
|
I think I am missing something, the contributing guide says to run integration tests on the The changes I made to the How are those created/updated? |
* add functions anythingButSuffix, anythingButWildcard, and anythingButEqualsIgnoreCase * change anythingButPrefix to support multiple values * implement unit tests * implement integration tests * update snapshots
| throw new Error('anythingBut matchers must be non-empty lists'); | ||
| } | ||
|
|
||
| return this.fromObjects([{ 'anything-but': { prefix: values } }]); |
There was a problem hiding this comment.
This will always output an array also when only a single value is supplied.
To avoid changes to existing rules created with this pattern, I can add an exception to handle such cases.
aws-cdk-automation
added
the
pr/needs-community-review
packages/@aws-cdk-testing/framework-integ/test/aws-events/test/integ.rule.ts
Show resolved
Hide resolved
|
It seems to be working well, but let me share the procedure just in case.
|
| @@ -109,8 +116,45 @@ export class Match implements IResolvable { | |||
| /** | |||
| * Matches any string that doesn't start with the given prefix. | |||
| */ | |||
| public static anythingButPrefix(prefix: string): string[] { | |||
| return this.fromObjects([{ 'anything-but': { prefix: prefix } }]); | |||
| public static anythingButPrefix(...values: string[]): string[] { | |||
There was a problem hiding this comment.
Changing an existing argument from a single string to string[] is a breaking change.
If you implement this, you would need to use feature flags, which would increase the complexity of the PR.
Please either avoid breaking changes as much as possible or modify the PR to use feature flags.
Example PRs:
There was a problem hiding this comment.
Is it a breaking change? If the function is called with a single value like anythingButPrefix('foo') then it still works as before, so it is backwards compatible.
Otherwise I can make it something like this:
public static anythingButPrefix(prefix: string, ...values: string[]): string[] {
values.unshift(prefix);
// rest of code
}There was a problem hiding this comment.
Another option would be to use function overloading:
public static anythingButPrefix(prefix: string): string[];
public static anythingButPrefix(...values: string[]): string[] {
return this.anythingButConjunction('prefix', values);
}There was a problem hiding this comment.
I'm really sorry for my misunderstandings.. This is not a breaking change and you don't need to update any more.
There was a problem hiding this comment.
No problem, thanks for taking the time to review.
What are the next step? Should I merge/rebase the latest changes from the main branch?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #32063 +/- ##
=======================================
Coverage 78.50% 78.50%
=======================================
Files 106 106
Lines 7201 7201
Branches 1321 1321
=======================================
Hits 5653 5653
Misses 1361 1361
Partials 187 187
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Thank you very much for your assistance!
In this PR, I believe not only the wildcard but also matches like equals-ignore-case have been newly added. Could you please make the following adjustments accordingly?
- Modify the PR title and description
- Add usage examples for all newly added public static methods in the README
aws-cdk-automation
removed
the
pr/needs-community-review
jaw111
changed the title
|
@badmintoncryer @mrgrain what are the next steps, is anything still needed from me? |
|
@jaw111 Could you please merge the main branch? Since this PR has passed community review, the `needs-maintainer-review' label will be attached and the maintainer would review this. |
aws-cdk-automation
added
the
pr/needs-maintainer-review
packages/@aws-cdk-testing/framework-integ/test/aws-events/test/integ.rule.ts
Show resolved
Hide resolved
Co-authored-by: Grace Luo <[email protected]>
mergify
bot
dismissed
gracelu0’s stale review
Pull request has been modified.
|
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
removed
the
pr/needs-maintainer-review
|
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). |
|
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). |
|
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). |
Issue # (if applicable)
Closes #28462.
Reason for this change
Add support for:
Extend anything-but matching on prefixes to support a list of prefix values.
Description of changes
Added functions on
Matchclass:prefixEqualsIgnoreCase()suffixEqualsIgnoreCase()wildcard()anythingButSuffix()anythingButWildcard()anythingButEqualsIgnoreCase()Modified
anythingButPrefix()to support rest parameters.Description of how you validated changes
Added unit and integration tests for new and changed functions.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license