migrate AWS SDK v2 → v3#686
Conversation
|
please fix merge conflicts and share e2e testing |
zjawol
force-pushed
the
chore/migrate-from-aws-sdk-v2-to-aws-sdk-v3
branch
from
c0ff691 to
53e2774
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR completes the migration from AWS SDK v2 to v3 by introducing a centralized ChangesAWS SDK v3 Migration & Feature Expansion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes The PR involves heterogeneous changes across multiple systems: a new AWS client factory abstraction with lazy loading and region overrides; wholesale refactoring of AWS operations from SDK v2 to v3 command patterns across six services; new CLI command implementations with esbuild integration; comprehensive property-based and integration test rewrites; and multi-part documentation. Each area requires separate reasoning about contract correctness, command payload construction, error handling semantics, and test coverage validity. Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/index.ts (1)
674-678:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t suppress all errors in
--quietmode.
src/index.tscurrently swallows anyErrorwhen--quietis set increateDomain(674-678),deleteDomain(694-698), andchangeRoute53Record(959-963). Limit quiet-mode suppression to idempotency-style failures only—e.g., AppSyncDeleteDomainName/GetDomainName:NotFoundException, and Route53ChangeResourceRecordSets:InvalidChangeBatch—and rethrow auth/throttling/permission/etc.Also,
envSetrejects empty-string values because it usesif (!key || !value); allowvalue === ''.For AppSync
CreateDomainNamewhen the domain already exists, add the concrete AWS service errorname/codefrom the API/SDK model and use that as the quiet-mode allowlist entry.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/index.ts` around lines 674 - 678, Update error handling so quiet mode only suppresses idempotency-style AWS errors and rethrows everything else: in createDomain, deleteDomain, and changeRoute53Record replace the blanket "if (error instanceof Error && this.options.quiet)" suppression with a check that inspects the AWS error "name"/"code" (e.g., NotFoundException for AppSync Get/DeleteDomainName and InvalidChangeBatch for Route53 ChangeResourceRecordSets) and only log+swallow when the error.code/name matches the allowlist; any other errors (auth, throttling, permissions, etc.) must be rethrown. For AppSync CreateDomainName add the concrete AWS service error name/code returned by the SDK to the quiet-mode allowlist so existing-domain errors are treated as idempotent. Lastly, in envSet change the falsy check "if (!key || !value)" to allow empty-string values (use a stricter test like value === undefined || value === null) so value === '' is permitted.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@doc/testing-resolvers.md`:
- Around line 25-27: The three fenced code blocks in doc/testing-resolvers.md
that currently lack language tags should be updated to include a language
identifier (use "text") so markdownlint MD040 passes; specifically add ```text
before the block containing "context → request() → [data source — not
called] → response() → result", the larger flow block starting with
"context" and ending with "result", and the file tree block beginning with
"functions/" and "tests/"; ensure you only add the language specifier (e.g.,
change ``` to ```text) for those blocks and leave the block content unchanged.
In `@src/__tests__/commands.test.ts`:
- Line 88: Tests currently use non-null assertions like createCall![0].input
after find()-style lookups which can be undefined; add a small reusable guard
helper (e.g., assertDefined<T>(value: T | undefined, msg?: string) or
getCommandCall(calls, idx)) and call it where the tests currently use `!` so the
value is checked at runtime and the TypeScript non-null assertion is removed.
Replace patterns such as `createCall![0].input` with a guarded form like `const
call = assertDefined(createCall, "createCall not found")[0];` (or
`getCommandCall(createCall, 0).input`) and reuse this helper for all listed
occurrences to keep tests type-safe and lint-clean.
In `@src/index.ts`:
- Around line 1271-1274: The current check in the envSet command uses a falsy
check (if (!key || !value)) which rejects empty-string values; change the
validation to explicitly test for undefined (e.g., key === undefined || value
=== undefined) so that "" is accepted as a valid value; update the condition
around the throw in the envSet handler (the block that throws new
this.serverless.classes.Error('You must specify both --key and --value.')) to
use the undefined check against the key and value variables.
In `@src/utils.ts`:
- Line 20: Prettier complains about the indexed access type formatting for
TimeUnit; update the type declaration so the indexed access applies to the type
of the array by wrapping typeof units in parentheses: change the declaration
referencing TimeUnit to use (typeof units)[number] (referencing the symbols
TimeUnit and units in this file).
---
Outside diff comments:
In `@src/index.ts`:
- Around line 674-678: Update error handling so quiet mode only suppresses
idempotency-style AWS errors and rethrows everything else: in createDomain,
deleteDomain, and changeRoute53Record replace the blanket "if (error instanceof
Error && this.options.quiet)" suppression with a check that inspects the AWS
error "name"/"code" (e.g., NotFoundException for AppSync Get/DeleteDomainName
and InvalidChangeBatch for Route53 ChangeResourceRecordSets) and only
log+swallow when the error.code/name matches the allowlist; any other errors
(auth, throttling, permissions, etc.) must be rethrown. For AppSync
CreateDomainName add the concrete AWS service error name/code returned by the
SDK to the quiet-mode allowlist so existing-domain errors are treated as
idempotent. Lastly, in envSet change the falsy check "if (!key || !value)" to
allow empty-string values (use a stricter test like value === undefined || value
=== null) so value === '' is permitted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4985f395-7d75-49cf-a075-c026646a87a5
⛔ Files ignored due to path filters (8)
package-lock.jsonis excluded by!**/package-lock.jsonsrc/__tests__/__snapshots__/api.test.ts.snapis excluded by!**/*.snapsrc/__tests__/__snapshots__/dataSources.test.ts.snapis excluded by!**/*.snapsrc/__tests__/__snapshots__/getAppSyncConfig.test.ts.snapis excluded by!**/*.snapsrc/__tests__/__snapshots__/waf.test.ts.snapis excluded by!**/*.snapsrc/__tests__/validation/__snapshots__/auth.test.ts.snapis excluded by!**/*.snapsrc/__tests__/validation/__snapshots__/base.test.ts.snapis excluded by!**/*.snapsrc/__tests__/validation/__snapshots__/resolvers.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (21)
README.mddoc/commands.mddoc/testing-resolvers.mdpackage.jsonsrc/__tests__/api.test.tssrc/__tests__/aws-client-factory.pbt.test.tssrc/__tests__/aws-client-factory.test.tssrc/__tests__/commands.test.tssrc/__tests__/dataSources.test.tssrc/__tests__/error-handling.pbt.test.tssrc/__tests__/getAppSyncConfig.test.tssrc/__tests__/index.test.tssrc/__tests__/js-resolvers.test.tssrc/__tests__/mapping-templates.test.tssrc/__tests__/resolvers.test.tssrc/__tests__/schema.test.tssrc/aws-client-factory.tssrc/get-stack-value.tssrc/index.tssrc/types/serverless.d.tssrc/utils.ts
💤 Files with no reviewable changes (1)
- src/get-stack-value.ts
| ``` | ||
| context → request() → [data source — not called] → response() → result | ||
| ``` |
There was a problem hiding this comment.
Add missing fenced code block languages to satisfy markdownlint (MD040).
Three fenced blocks are missing language identifiers, which can fail docs linting.
Suggested fix
-```
+```text
context → request() → [data source — not called] → response() → result- +text
context
│
▼
[pipeline request handler] ← optional JS wrapper
│
▼
function 1: request() → [data source — not called] → response()
│ (result stored in ctx.prev.result, stash carried forward)
▼
function 2: request() → [data source — not called] → response()
│
▼
[pipeline response handler] ← optional JS wrapper
│
▼
result
-```
+```text
functions/
validateInput.js
savePost.js
tests/
createPost/
step1-validateInput-request.ctx.json ← input context
step1-validateInput-request.expected.json ← expected output
step2-validateInput-response.ctx.json
step2-validateInput-response.expected.json
step3-savePost-request.ctx.json
step3-savePost-request.expected.json
</details>
Also applies to: 53-70, 136-148
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 25-25: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @doc/testing-resolvers.md around lines 25 - 27, The three fenced code blocks
in doc/testing-resolvers.md that currently lack language tags should be updated
to include a language identifier (use "text") so markdownlint MD040 passes;
specifically add text before the block containing "context → request() → [data source — not called] → response() → result", the larger flow block starting with "context" and ending with "result", and the file tree block beginning with "functions/" and "tests/"; ensure you only add the language specifier (e.g., change to ```text) for those blocks and leave the block
content unchanged.
</details>
<!-- fingerprinting:phantom:triton:hawk -->
<!-- This is an auto-generated comment by CodeRabbit -->
|
|
||
| expect(createCall).toBeDefined(); | ||
| expect(listCertCall).toBeUndefined(); | ||
| expect(createCall![0].input).toMatchInlineSnapshot(` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all remaining non-null assertions in this test file after refactor.
rg -nP '!\s*(\[|\.|;|,|\))' src/__tests__/commands.test.tsRepository: sid88in/serverless-appsync-plugin
Length of output: 1469
Remove non-null assertions from command lookup results.
find()-style lookups can yield undefined; this test file still uses ! non-null assertions on command call results at lines 88, 150, 257, 287, 319, 399, 497, 557, 616, 675, 778, 783, 804, 997, 1002, 1023, 1124, 1129, 1150, 1339, 1485, and 1530. Introduce a single guard/helper and reuse it so these assertions stay type-safe and lint-clean.
Proposed refactor pattern
+function requireCommandCall<T>(
+ call: [T] | undefined,
+ commandName: string,
+): [T] {
+ expect(call).toBeDefined();
+ if (!call) throw new Error(`Expected ${commandName} to be sent`);
+ return call;
+}
...
- expect(createCall![0].input).toMatchInlineSnapshot(...)
+ const [createCmd] = requireCommandCall(createCall, 'CreateDomainNameCommand');
+ expect(createCmd.input).toMatchInlineSnapshot(...)🧰 Tools
🪛 GitHub Check: tests (20)
[warning] 88-88:
Forbidden non-null assertion
🪛 GitHub Check: tests (22)
[warning] 88-88:
Forbidden non-null assertion
🪛 GitHub Check: tests (24)
[warning] 88-88:
Forbidden non-null assertion
🪛 GitHub Check: tests (26)
[warning] 88-88:
Forbidden non-null assertion
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/__tests__/commands.test.ts` at line 88, Tests currently use non-null
assertions like createCall![0].input after find()-style lookups which can be
undefined; add a small reusable guard helper (e.g., assertDefined<T>(value: T |
undefined, msg?: string) or getCommandCall(calls, idx)) and call it where the
tests currently use `!` so the value is checked at runtime and the TypeScript
non-null assertion is removed. Replace patterns such as `createCall![0].input`
with a guarded form like `const call = assertDefined(createCall, "createCall not
found")[0];` (or `getCommandCall(createCall, 0).input`) and reuse this helper
for all listed occurrences to keep tests type-safe and lint-clean.
| if (!key || !value) { | ||
| throw new this.serverless.classes.Error( | ||
| 'You must specify both --key and --value.', | ||
| ); |
There was a problem hiding this comment.
Allow empty-string env values in envSet.
if (!key || !value) rejects --value "", which can be a valid update. Check for undefined instead of truthiness.
Suggested fix
- if (!key || !value) {
+ if (key === undefined || value === undefined) {
throw new this.serverless.classes.Error(
'You must specify both --key and --value.',
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!key || !value) { | |
| throw new this.serverless.classes.Error( | |
| 'You must specify both --key and --value.', | |
| ); | |
| if (key === undefined || value === undefined) { | |
| throw new this.serverless.classes.Error( | |
| 'You must specify both --key and --value.', | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/index.ts` around lines 1271 - 1274, The current check in the envSet
command uses a falsy check (if (!key || !value)) which rejects empty-string
values; change the validation to explicitly test for undefined (e.g., key ===
undefined || value === undefined) so that "" is accepted as a valid value;
update the condition around the throw in the envSet handler (the block that
throws new this.serverless.classes.Error('You must specify both --key and
--value.')) to use the undefined check against the key and value variables.
|
Thanks for this, @zjawol — the v3 migration is a solid piece of work and it's been on our list for a while. I reviewed it end to end and pushed a few follow-up commits on top; summary below for posterity. What this PR does Verified (offline)
Follow-up commits added during review
Non-blocking follow-ups for later: the six CI is green. Thanks again, @zjawol — merging. 🎉 |
Sibling to the offline e2e suite, exercising the plugin's live AWS code paths (post AWS-SDK-v3 migration in #686) against a real account. Opt-in via APPSYNC_PLUGIN_INTEGRATION=1; skips cleanly and is excluded from npm test / test:e2e / test:all and default CI. - Tier A: appsync evaluate (VTL + JS), no deploy - Tier B: minimal deploy -> info/get-introspection/env/logs + the credential/region proof for #686 - Tier C: caching/flush-cache (gated) - Tier D: custom domain + Route53 + ACM (gated, off by default) - run-id tagging + serverless remove teardown + standalone sweeper - doc/integration-tests.md, optional OIDC workflow (manual/scheduled)
2 participants
Proposed changes
Migrates all AWS SDK usage from the legacy
aws-sdkv2 package to the modular AWS SDK v3 (@aws-sdk/*packages). This is a prerequisite for dropping the v2 dependency and aligns the plugin with the current AWS SDK standard.Key changes:
AwsClientFactory— new class (src/aws-client-factory.ts) that lazily initializes and caches SDK v3 clients (AppSync, CloudFormation, CloudWatch Logs, Route53, ACM). ACM client is always pinned tous-east-1as required by CloudFront.src/index.ts— all AWS API calls migrated from v2 SDK classes/types to v3*Commandpattern.AwsClientFactoryis instantiated once in the constructor usingfromNodeProviderChain()for credentials.src/get-stack-value.ts— removed (dead code, functionality covered elsewhere).sls appsync evaluate— new CLI command that calls the AppSyncEvaluateCodeandEvaluateMappingTemplateAPIs to test JS resolvers and VTL templates against a context without deploying.sls appsync env get/set— new CLI commands to read and update AppSync runtime environment variables on a deployed API without redeploying.README.mdanddoc/commands.mdwith quick-reference tables; addeddoc/testing-resolvers.mdcovering theevaluateworkflow, fixture structure, pipeline chaining, and Jest integration examples. Node.js minimum requirement updated from v16 to v20.Issue(s)
Closes #685
Steps to test or reproduce
sls appsync evaluate --type Query --field getUser --function request \ --context '{"arguments":{"id":"abc-123"}}'sls appsync env get sls appsync env set --key STAGE --value productionSummary by CodeRabbit
Release Notes
New Features
sls appsync evaluatecommand for resolver evaluation (JavaScript and VTL modes).sls appsync env getandenv setcommands for environment variable management.Documentation