refactor: provider-cloudformation and amplify-category-storage #14238
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
packages/amplify-provider-awscloudformation/src/aws-utils/DynamoDBService.ts
Fixed
Show fixed
Hide fixed
packages/amplify-provider-awscloudformation/src/system-config-manager.ts
Fixed
Show fixed
Hide fixed
| } | ||
| return reject(updateErr); | ||
| } | ||
| cfnModel.waitFor(cfnCompleteStatus, cfnStackCheckParams, async (completeErr) => { |
There was a problem hiding this comment.
need some additional feedback on this section
| args.push('--debug'); | ||
|
|
There was a problem hiding this comment.
Here and in other places.
I'm assuming this will get removed in final version of PR.
In general we should be running tests without it. To make sure that command without it works as expected (there's always small risk that debug flag might alter logic - if that happens it would be unwanted regression).
For test debugging the alternative is to set env var mentioned here
.in the workflow definition or codebuild build. (there's probably no automation/runbook for it yet, but should be possible).
There was a problem hiding this comment.
yep, if you see any --debug or console.log statements, those are relics from testing
| @@ -70,7 +79,7 @@ export class Lambda { | |||
| } | |||
|
|
|||
| try { | |||
| await Promise.all(deletionPromises); | |||
| await Promise.all(deletionPromises.map((fn) => fn())); | |||
There was a problem hiding this comment.
maybe not, I'll check
| const streamToString = async (stream: Readable) => { | ||
| const chunks = []; | ||
| for await (const chunk of stream) { | ||
| chunks.push(Buffer.from(chunk)); | ||
| } | ||
| return Buffer.concat(chunks).toString('utf-8'); | ||
| }; | ||
|
|
There was a problem hiding this comment.
There is import { text } from 'node:stream/consumers'; that could be used for this.
There was a problem hiding this comment.
'node:stream/consumers' only works with node versions 16+, we have some packages that have @types/node set to 12, which seem to be blocking us from using this
There was a problem hiding this comment.
you can try upgrading @types/node. The runtimes we're using now are 20+.
Description of changes
Migrating:
Something you may notice in this PR is there are some new functions that convert streams to something else, this is because the Body field within
GetObjectCommandis now aReadableStream, where as ingetObjectit had a more flexible type which allowed it to be astringor aBufferas well as a stream.Changes the way credentials are handled to use the V3-style object instead of the V2-style one
This includes a lot of high risk changes, will need additional testing
Issue #, if available
Description of how you validated changes
Checklist
yarn testpassesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.