Skip to content

Conversation

@jmnarloch
Copy link

@jmnarloch jmnarloch commented Feb 24, 2020


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

Summary

This is initial code review for supporting the EventBridge Content Filtering capabilities.

Issue: #6184

Highlights:

@mergify
Copy link
Contributor

mergify bot commented Feb 24, 2020

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

@jmnarloch jmnarloch force-pushed the content-filtering branch 2 times, most recently from 1e20493 to 3015f87 Compare February 24, 2020 01:29
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 5f18ee6
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 1e20493
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 3015f87
  • Result: FAILED
  • Build Logs (available for 30 days)

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

* Matches event based attribute value.
* @param values possible values to match the event attribute against
*/
public static match(...values: string[]): any[] {
Copy link
Author

Choose a reason for hiding this comment

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

Alternatively instead of prefixing each method with match I was thinking about using just the matcher name.

This way matchPrefix() would become prefix(), matchAnythingBut() would be anythingBut() and so on.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you rename the class as well, I think Match.prefix() would look pretty neat.

* @default - No filtering on version
*/
readonly version?: string[];
readonly version?: any[];
Copy link
Author

Choose a reason for hiding this comment

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

The big question is whether this is a backward compatible change across all CDK supported languages?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it will be. We probably shouldn't be doing this.

A solution is to use Tokens to force the output of the matcher functions to strings.

/**
* Event Pattern matcher.
*/
export abstract class Matcher {
Copy link
Author

@jmnarloch jmnarloch Feb 24, 2020

Choose a reason for hiding this comment

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

The other option instead of introducing dedicated factory class is to include this functionality as part of EventPattern type.

* Matches event based on presence or absence of attribute.
* @param value whether the attribute should exist or not
*/
public static matchExists(value: boolean): any[] {
Copy link
Author

Choose a reason for hiding this comment

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

Does CDK's jsii allows to define default values for parameters? Like in this case value: boolean = true

Copy link
Contributor

Choose a reason for hiding this comment

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

It does.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: e923cfa
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: c94cc2c
  • Result: FAILED
  • Build Logs (available for 30 days)

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

…aws#6184)

Adds dedicated EventPattern factory methods for building the pattern.
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: d439d18
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

* @default - No filtering on version
*/
readonly version?: string[];
readonly version?: any[];
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it will be. We probably shouldn't be doing this.

A solution is to use Tokens to force the output of the matcher functions to strings.

* Matches event based attribute value.
* @param values possible values to match the event attribute against
*/
public static match(...values: string[]): any[] {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you rename the class as well, I think Match.prefix() would look pretty neat.

* Matches event based on presence or absence of attribute.
* @param value whether the attribute should exist or not
*/
public static matchExists(value: boolean): any[] {
Copy link
Contributor

Choose a reason for hiding this comment

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

It does.

@mergify
Copy link
Contributor

mergify bot commented Mar 25, 2020

Once all the requested changes have been addressed, and the PR is ready for another review, remember to dismiss the review.

@DavidChristiansen
Copy link
Contributor

Hi @jmnarloch are you going to be working on this or is it dead.

@rix0rrr rix0rrr removed their assignment Jun 22, 2021
@cgarvis cgarvis added p2 response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. labels Mar 10, 2022
@github-actions
Copy link
Contributor

This PR has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added closing-soon This issue will automatically close in 4 days unless further comments are made. closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Mar 16, 2022
@github-actions github-actions bot closed this Mar 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. p2 response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants