feat(events): add filter rules for prefixEqualsIgnoreCase, suffixEqualsIgnoreCase, wildcard, and anythingBut* matches#32063
feat(events): add filter rules for prefixEqualsIgnoreCase, suffixEqualsIgnoreCase, wildcard, and anythingBut* matches#32063mergify[bot] merged 28 commits intoaws:mainfrom jaw111:28462-aws-events-match-wildcard
Conversation
|
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.
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.
|
| */ | ||
| 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.67% 78.67%
=======================================
Files 107 107
Lines 7237 7237
Branches 1329 1329
=======================================
Hits 5694 5694
Misses 1357 1357
Partials 186 186
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
|
@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. |
gracelu0
left a comment
There was a problem hiding this comment.
A few minor comments, otherwise LGTM! Once addressed I'll approve.
packages/@aws-cdk-testing/framework-integ/test/aws-events/test/integ.rule.ts
Show resolved
Hide resolved
Co-authored-by: Grace Luo <54298030+gracelu0@users.noreply.github.com>
Pull request has been modified.
gracelu0
left a comment
There was a problem hiding this comment.
Thank you for contributing!
|
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). |
|
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). |
|
@gracelu0 Looks like the codecov check is broken here too, which is preventing this PR from merging (and because of the queue it's also preventing other PRs from merging). |
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). |
|
Comments on closed issues and PRs are hard for our team to see. |
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