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

S3 notifications with multiple filters not intuitive/error prone #3347

Closed
2 of 5 tasks
jogold opened this issue Jul 18, 2019 · 0 comments Β· Fixed by #3397
Closed
2 of 5 tasks

S3 notifications with multiple filters not intuitive/error prone #3347

jogold opened this issue Jul 18, 2019 · 0 comments Β· Fixed by #3397
Labels
needs-triage This issue or PR still needs to be triaged.

Comments

@jogold
Copy link
Contributor

jogold commented Jul 18, 2019

  • I'm submitting a ...

    • πŸͺ² bug report
    • πŸš€ feature request
    • πŸ“š construct library gap
    • ☎️ security issue or vulnerability => Please see policy
    • ❓ support request => Please see note at the top of this template.
  • What is the current behavior?
    The following code fails at deploy time (CF) with the following error message from the custom resource Cannot specify more than one prefix rule in a filter.

const stack = new cdk.Stack();
const bucket = new s3.Bucket(stack, 'MyBucket');

bucket.addObjectCreatedNotification(
  new notifications.LambdaDestination(notifierFn),
  { prefix: 'a/' },
  { prefix: 'b/' }
)

This is because FilterRules in putBucketNotificationConfiguration expects a list of at most two different elements (one prefix and one suffix).

Since the following is possible:

bucket.addObjectCreatedNotification(
  new notifications.LambdaDestination(notifierFn),
  { prefix: 'a/' , suffix: '.txt' }
)

and equivalent to

bucket.addObjectCreatedNotification(
  new notifications.LambdaDestination(notifierFn),
  { prefix: 'a/' }, 
  { suffix: '.txt' }
)

There's actually no need for multiple filters with the current implementation...

The CDK can/should do better than this...

  • What is the expected behavior (or behavior of feature suggested)?
    Throw when specifying multiple prefixes/suffixes.

Ideally, the following API should be possible:

bucket.addObjectCreatedNotification(
  new notifications.LambdaDestination(notifierFn),
  { prefix: 'docs/', suffix: '.pdf' },
  { prefix: 'images/', suffix: '.png' }
)

(creates, behind the scene, multiple LambdaFunctionConfigurations (or other configurations types) for the user)

  • What is the motivation / use case for changing the behavior or adding this feature?
    Avoid errors, fail early, offer a user friendly API

  • Please tell us about your environment:

    • CDK CLI Version: 1.0.0
    • Module Version: 1.0.0
    • OS: all
    • Language: all
  • Other information (e.g. detailed explanation, stacktraces, related issues, suggestions how to fix, links for us to have context, eg. associated pull-request, stackoverflow, gitter, etc)

@jogold jogold added the needs-triage This issue or PR still needs to be triaged. label Jul 18, 2019
jogold added a commit to jogold/aws-cdk that referenced this issue Jul 23, 2019
Avoid CF deploy time errors when specifying multiple prefixes or suffixes in notification filters.

Closes aws#3347
eladb pushed a commit that referenced this issue Jul 23, 2019
Avoid CF deploy time errors when specifying multiple prefixes or suffixes in notification filters.

Closes #3347
Possibly something to fix in v2.0 (#3398)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-triage This issue or PR still needs to be triaged.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant