feat(s3): update runtime of notifications-handler to python3.9 #23209

watany-dev · 2022-12-02T16:28:17Z

closes #20973

This upgrades the runtime of the internal function notifications-resource-handler from Python 3.7 to Python 3.9

Referring to the PR below
#21483

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Construct Runtime Dependencies:

This PR adds new construct runtime dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

gitpod-io · 2022-12-02T16:28:26Z

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

watany-dev · 2022-12-02T16:33:47Z

I have updated the existing integ-test and verified that it is the updated runtime. Do I need to create any more new tests?

watany-dev · 2022-12-02T16:56:39Z

The updated integ test seems to have an error if the new module is not used. Will address this tomorrow.

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

mrgrain · 2022-12-05T14:49:55Z

Code looks good. I checked and python3.9 is available everywhere we need it.

Once the test passes, this should be good to ship.

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

watany-dev · 2022-12-05T15:38:35Z

Glad to have your review.
I checked and it looks like the integ test needed @s3-notification as well as @s3. I'm currently waiting for the results of a push that resolves this.

watany-dev · 2022-12-06T01:06:31Z

@mrgrain
We were able to confirm the PASS of all tests.

mrgrain

Just a minor thing.

But I'll need some more time to look at these integ tests. I'm a bit surprised so many snapshots need updating.

mrgrain · 2022-12-06T18:52:09Z

packages/@aws-cdk/aws-lambda-event-sources/lib/kafka.ts

-import { md5hash } from '@aws-cdk/core/lib/helpers-internal';
 import { ISecurityGroup, IVpc, SubnetSelection } from '@aws-cdk/aws-ec2';
 import * as iam from '@aws-cdk/aws-iam';
 import * as lambda from '@aws-cdk/aws-lambda';
 import * as secretsmanager from '@aws-cdk/aws-secretsmanager';
 import { Stack, Names } from '@aws-cdk/core';
+import { md5hash } from '@aws-cdk/core/lib/helpers-internal';


Would you mind reverting this unrelated change. Thanks.

This error was automatically fixed by yarn build --fix before integ-test.
Do I still need to fix it now that the test has passed because the IDE outputs the following Warning.

Import of `@aws-cdk/core/lib/helpers-internal` must be done after import of `@aws-cdk/core`.

Nevertheless, I have added the commit to be corrected as requested.

Ah I see, thanks for explaining where this came from. 👍🏻 Probably would have been fine to leave it in then. From a PR review it's difficult to establish the source of these kind of changes (and manually checking would just take to long).

Sorry I was in a hurry to fix it. This commit should bring us back to the beginning of this review!

All good and thank you for being diligent. It's a tricky thing. When the same module has other change I usually don't question it.

Maybe adding a comment to the line on GitHub would help reviewers. 🤔

Pull request has been modified.

mrgrain · 2022-12-07T14:59:01Z

This looks all good. I'm currently looking into adding an extended test that actually tests the notification delivery. Not sure if it's possible though.

mrgrain

🚀

mergify · 2022-12-07T17:48:35Z

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 · 2022-12-07T19:19:43Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: efdfd67
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2022-12-07T19:19:57Z

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).

…3209) closes aws#20973 This upgrades the runtime of the internal function `notifications-resource-handler` from Python 3.7 to Python 3.9 Referring to the PR below aws#21483 ---- ### All Submissions: * [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Construct Runtime Dependencies: * [ ] This PR adds new construct runtime dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

github-actions bot added the p1 label Dec 2, 2022

aws-cdk-automation requested a review from a team December 2, 2022 16:28

github-actions bot added the admired-contributor [Pilot] contributed between 13-24 PRs to the CDK label Dec 2, 2022

aws-cdk-automation previously requested changes Dec 2, 2022

View reviewed changes

watany-dev changed the title ~~fix(s3): Runtime modernization of notifications-handler~~ fix(s3): Upgrade Runtime to Python 3.9 of notifications-handler Dec 2, 2022

watany-dev force-pushed the bucket-noti-py3.9 branch from 415f08f to 01907d3 Compare December 5, 2022 13:56

watany-dev force-pushed the bucket-noti-py3.9 branch from 01907d3 to bfac3a0 Compare December 5, 2022 14:29

mrgrain self-assigned this Dec 5, 2022

mrgrain changed the title ~~fix(s3): Upgrade Runtime to Python 3.9 of notifications-handler~~ fix(s3): upgrade Runtime to Python 3.9 of notifications-handler Dec 5, 2022

mrgrain changed the title ~~fix(s3): upgrade Runtime to Python 3.9 of notifications-handler~~ feat(s3): update runtime of notifications-handler to python3.9 Dec 5, 2022

aws-cdk-automation previously requested changes Dec 5, 2022

View reviewed changes

mrgrain added the pr-linter/exempt-readme The PR linter will not require README changes label Dec 5, 2022

mrgrain added p2 and removed p1 labels Dec 5, 2022

fix(s3): Runtime modernization of notifications-handler

61fa3bc

watany-dev force-pushed the bucket-noti-py3.9 branch from bfac3a0 to 61fa3bc Compare December 5, 2022 15:30

integ lambda-event-sources

b4fd754

mrgrain previously requested changes Dec 6, 2022

View reviewed changes

import order

c156ab5

watany-dev requested a review from mrgrain December 7, 2022 02:20

retake import order

76e5711

mrgrain added 2 commits December 7, 2022 17:10

update integ test snapshot

f4e1fb9

extend test to check notification is received by queue

c423552

mrgrain approved these changes Dec 7, 2022

View reviewed changes

Merge branch 'main' into bucket-noti-py3.9

efdfd67

mergify bot merged commit b2d293d into aws:main Dec 7, 2022

watany-dev deleted the bucket-noti-py3.9 branch December 7, 2022 22:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(s3): update runtime of notifications-handler to python3.9 #23209

feat(s3): update runtime of notifications-handler to python3.9 #23209

watany-dev commented Dec 2, 2022

gitpod-io bot commented Dec 2, 2022

aws-cdk-automation left a comment

watany-dev commented Dec 2, 2022 •

edited

watany-dev commented Dec 2, 2022

mrgrain commented Dec 5, 2022

aws-cdk-automation left a comment

watany-dev commented Dec 5, 2022

watany-dev commented Dec 6, 2022

mrgrain left a comment

mrgrain Dec 6, 2022

watany-dev Dec 6, 2022

watany-dev Dec 7, 2022

mrgrain Dec 7, 2022

watany-dev Dec 7, 2022

mrgrain Dec 7, 2022

mrgrain commented Dec 7, 2022

mrgrain left a comment

mergify bot commented Dec 7, 2022

aws-cdk-automation commented Dec 7, 2022

mergify bot commented Dec 7, 2022

feat(s3): update runtime of notifications-handler to python3.9 #23209

feat(s3): update runtime of notifications-handler to python3.9 #23209

Conversation

watany-dev commented Dec 2, 2022

All Submissions:

Adding new Construct Runtime Dependencies:

New Features

gitpod-io bot commented Dec 2, 2022

aws-cdk-automation left a comment

Choose a reason for hiding this comment

watany-dev commented Dec 2, 2022 • edited

watany-dev commented Dec 2, 2022

mrgrain commented Dec 5, 2022

aws-cdk-automation left a comment

Choose a reason for hiding this comment

watany-dev commented Dec 5, 2022

watany-dev commented Dec 6, 2022

mrgrain left a comment

Choose a reason for hiding this comment

mrgrain Dec 6, 2022

Choose a reason for hiding this comment

watany-dev Dec 6, 2022

Choose a reason for hiding this comment

watany-dev Dec 7, 2022

Choose a reason for hiding this comment

mrgrain Dec 7, 2022

Choose a reason for hiding this comment

watany-dev Dec 7, 2022

Choose a reason for hiding this comment

mrgrain Dec 7, 2022

Choose a reason for hiding this comment

mrgrain commented Dec 7, 2022

mrgrain left a comment

Choose a reason for hiding this comment

mergify bot commented Dec 7, 2022

aws-cdk-automation commented Dec 7, 2022

AWS CodeBuild CI Report

mergify bot commented Dec 7, 2022

watany-dev commented Dec 2, 2022 •

edited