feat(s3): onCloudTrailWriteObject matches all update events

[njlaw]

In addition to PutObject, Bucket.onCloudTrailPutObject() should also
match on event names CopyObject and CompleteMultipartUpload;
otherwise, the event does not trigger when files are uploaded using
those APIs. E.g., larger files are uploaded using the multipart API.

This fixes #4634

---

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

[mergify]

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

• PR title type(scope): text
  • type: fix, feat, refactor go into CHANGELOG, chore is hidden
  • scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
  • text: use all lower-case, do not end with a period, do not include issue refs
• PR Description
  • Rationale: describe rationale of change and approach taken
  • Issues: indicate issues fixed via: fixes #xxx or closes #xxx
  • Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
• Testing
  • Unit test added. Prefer to add a new test rather than modify existing tests
  • CLI or init templates change? Re-run/add CLI integration tests
• Documentation
  • README: update module README to describe new features
  • API docs: public APIs must be documented. Copy from official AWS docs when possible
  • Design: for significant features, follow design process

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: FAILED
• Build Logs (available for 30 days)

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

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: FAILED
• Build Logs (available for 30 days)

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

[njlaw]

When the commit ends up affecting other integration tests like this one, should I go ahead and run npm run integ for the affected tests? Or what is the proper process for that? I wasn't sure if I should be updating tests outside of aws-s3 for this.

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[eladb]

I am not sure I am comfortable with paggybacking these two additional events on onCloudTrailPutObject since the name of this method is very explicit. Maybe we can simply add an additional method onCloudTrailUploadObject (or onCloudTrailWriteObject) which has a more high-level name which will express this idea better, but will also allow people to use the more fine-grained ...PutObject.

What do you think?

[njlaw]

I am not sure I am comfortable with paggybacking these two additional events on onCloudTrailPutObject since the name of this method is very explicit. Maybe we can simply add an additional method onCloudTrailUploadObject (or onCloudTrailWriteObject) which has a more high-level name which will express this idea better, but will also allow people to use the more fine-grained ...PutObject.

What do you think?

I think creating a new method makes sense from the semantic point of view and agree that the name could be confusing if piggybacked.

I will create a new method onCloudTrailWriteObject and leave onCloudTrailPutObject alone. That way the functionality of the latter will be preserved.

As the issue was originally opened as a CodePipeline S3 source action issue, should I also update the bound() method there to use onCloudTrailWriteObject or would a separate PR would be better for that?

Pull request has been modified.

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[eladb]

should I also update the bound() method there to use onCloudTrailWriteObject or would a separate PR would be better for that?

Of course!

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[njlaw]

should I also update the bound() method there to use onCloudTrailWriteObject or would a separate PR would be better for that?

Of course!

Done! Thank you.

It looks like master is changing faster than CodeBuild CI will run, so I'll leave that for you or merge it again when the changes slow down.

[skinny85]

I am not sure I am comfortable with paggybacking these two additional events on onCloudTrailPutObject since the name of this method is very explicit. Maybe we can simply add an additional method onCloudTrailUploadObject (or onCloudTrailWriteObject) which has a more high-level name which will express this idea better, but will also allow people to use the more fine-grained ...PutObject.

Do we have in mind why would someone want to react on PutObject, but not on all of the other methods that you can use to insert objects into buckets...? I think we might be setting up people for disappointment by using onCloudTrailPutObject instead of onCloudTrailWriteObject...

Should we at least deprecate onCloudTrailPutObject(), pointing people to use onCloudTrailWriteObject() instead?

[eladb]

Perfect!

[mergify]

Thank you for contributing! Your pull request is now being automatically merged.

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[joekiller]

thanks everyone