Skip to content

fix: update awaiter s3 key #2732

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Aug 1, 2024
Merged

fix: update awaiter s3 key #2732

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...src/__tests__/provider-utils/awscloudformation/__snapshots__/ecs-apigw-stack.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ Object {
"Type": "String",
},
"awaiterS3Key": Object {
"Default": "custom-resource-pipeline-awaiter.zip",
"Default": "custom-resource-pipeline-awaiter-18.zip",
"Type": "String",
},
"deploymentBucketName": Object {
Expand Down
2 changes: 1 addition & 1 deletion packages/amplify-category-api/src/provider-utils/awscloudformation/base-api-stack.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import { NETWORK_STACK_LOGICAL_ID } from '../../category-constants';
import Container from './docker-compose/ecs-objects/container';
import { GitHubSourceActionInfo, PipelineWithAwaiter } from './pipeline-with-awaiter';

const PIPELINE_AWAITER_ZIP = 'custom-resource-pipeline-awaiter.zip';
const PIPELINE_AWAITER_ZIP = 'custom-resource-pipeline-awaiter-18.zip';
Copy link
Contributor

@sdstolworthy sdstolworthy Jul 31, 2024

Choose a reason for hiding this comment

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

Does this need to be parameterized? Under what conditions would this number need to change in the future? What is 18 referring to?

Copy link
Contributor Author

@dpilch dpilch Jul 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This name would need to change anytime this file is modified in the CLI repo. 18 is referring the node runtime version. The zip file was previously updated to be compatible with Node 18.

I think parametrization would be possible but I think would require larger changes to the API category and the CLI.

I chose the option to simply update the name (which will update the S3 key) because it would be the fastest method to release a fix for this while still being effective.

Copy link
Contributor

@phani-srikar phani-srikar Jul 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is the name of the key as stored in S3 by the CLI and this naming convention is shared b/w CLI and API plugins reference with the assumption on CLI side that the cloudformation provider would upload the lambda code in the deployment bucket using that key and we use it as part of the code pipeline state machine in our REST API deployments.

Copy link
Contributor

@sdstolworthy sdstolworthy Jul 31, 2024

Choose a reason for hiding this comment

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

Node 18 is EOL in 9 months. How do we prevent missing this 9 months from now? https://github.com/nodejs/Release

Copy link
Contributor

@phani-srikar phani-srikar Jul 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This key doesn't require a change for every Lambda runtime update but when the underlying code is not compatible with older versions which coincidentally is the case from 16.x -> 18.x runtime since lambda changed it's default support of AWS SDK version from 2.x to 3.x as part of this runtime update reference. We should have an E2E test added to the migration tests that CLI has so that we can catch such cases where a runtime update requires code change. We could do this as a follow-up after the fix is verified to work for this case.

Copy link
Contributor

@sdstolworthy sdstolworthy Jul 31, 2024

Choose a reason for hiding this comment

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

Sounds good. I think an e2e validation after the fact sounds like a good plan.


export enum DEPLOYMENT_MECHANISM {
/**
Expand Down