Migrate CI/CD from CodePipeline to GitHub Actions#193
Migrate CI/CD from CodePipeline to GitHub Actions#193
Conversation
Replace AWS CDK Pipelines (CodePipeline V2) with a GitHub Actions workflow for building and deploying. Stacks are now instantiated directly in app.ts with explicit stackName properties matching the existing CloudFormation stack names, enabling zero-diff takeover. - Rewrite lib/app.ts to instantiate ServiceStack and MonitoringStack directly for Beta and Prod stages - Change Smithy CLI download URL from aarch64 to x86_64 for GitHub Actions runners - Add .github/workflows/deploy.yml with build → beta → prod pipeline, concurrency queuing, and workflow_dispatch stage selector
|
Claude finished @layertwo's task in 2m 12s —— View job Code Review CompleteI've completed a comprehensive security and quality review of this PR that migrates from AWS CodePipeline to GitHub Actions. Review ChecklistCore Quality
Security
OWASP Top 10
Documentation
Issues SummaryMEDIUM Severity (2 issues)
LOW Severity (4 issues)
Security Rating: ✅ ACCEPTABLENo critical or high-severity security issues found. The MEDIUM issues are related to operational safety rather than direct security vulnerabilities. The migration follows GitHub Actions security best practices with OIDC authentication and proper secret management. RecommendationThis PR is approved for merge after addressing:
The architecture change from CodePipeline to GitHub Actions is sound and follows infrastructure-as-code best practices. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| - name: Install Smithy CLI | ||
| run: | | ||
| curl -L https://github.com/smithy-lang/smithy/releases/download/1.67.0/smithy-cli-linux-x86_64.zip -o smithy-cli.zip |
There was a problem hiding this comment.
MEDIUM - Security: Unverified External Download
The Smithy CLI is downloaded from GitHub without checksum verification. An attacker could potentially serve a malicious binary if the GitHub release is compromised or through a MITM attack.
Recommendation: Add SHA256 checksum verification after download:
- name: Install Smithy CLI
run: |
curl -L https://github.com/smithy-lang/smithy/releases/download/1.67.0/smithy-cli-linux-x86_64.zip -o smithy-cli.zip
echo "EXPECTED_SHA256_HERE smithy-cli.zip" | sha256sum -c -
unzip -qo smithy-cli.zip -d smithy-install
sudo smithy-install/smithy-cli-linux-x86_64/installYou can obtain the checksum from the GitHub release page or by running sha256sum on a trusted download.
| aws-region: ${{ env.AWS_REGION }} | ||
|
|
||
| - name: Deploy Beta-ServiceStack | ||
| run: npx cdk deploy Beta-ServiceStack --app build/cdk.out --require-approval never |
There was a problem hiding this comment.
LOW - Deployment Safety: No Rollback Strategy
The deployment steps use --require-approval never which bypasses CDK's safety checks. If a deployment fails midway, there's no automated rollback mechanism.
Recommendation: Consider:
- Adding
--rollbackflag support (available in newer CDK versions) - Adding a post-deployment verification step
- Using
--progress eventsfor better visibility into deployment progress
Example:
- name: Deploy Beta-ServiceStack
run: npx cdk deploy Beta-ServiceStack --app build/cdk.out --require-approval never --progress events
| github.event_name == 'push' || | ||
| github.event.inputs.stage == 'all' || | ||
| github.event.inputs.stage == 'prod' | ||
| needs: [build, deploy-beta] |
There was a problem hiding this comment.
MEDIUM - Deployment Logic: Missing Beta Deployment Condition for workflow_dispatch
When using workflow_dispatch with stage: prod, the prod deployment will fail because it has needs: [build, deploy-beta], but deploy-beta will be skipped (its if condition won't match).
Current behavior:
- Manual trigger with
stage: prod→ beta skipped → prod fails (missing dependency)
Recommendation: Update the dependency logic:
deploy-prod:
if: >-
github.event_name == 'push' ||
github.event.inputs.stage == 'all' ||
github.event.inputs.stage == 'prod'
needs: build
# Remove deploy-beta from needs, or make it conditionalOr change line 104 to: needs: [build] and add explicit stack ordering within the job if beta must deploy first.
| // renovate: datasource=github-releases depName=smithy-lang/smithy | ||
| export const SMITHY_LANG_VER = "1.67.0"; | ||
| export const SMITHY_DOWNLOAD_URL = `https://github.com/smithy-lang/smithy/releases/download/${SMITHY_LANG_VER}/smithy-cli-linux-aarch64.zip`; | ||
| export const SMITHY_DOWNLOAD_URL = `https://github.com/smithy-lang/smithy/releases/download/${SMITHY_LANG_VER}/smithy-cli-linux-x86_64.zip`; |
There was a problem hiding this comment.
LOW - Code Quality: Unused Export
The SMITHY_DOWNLOAD_URL constant is defined but not used anywhere in the codebase. The workflow file hardcodes the URL directly (line 40 of deploy.yml).
Recommendation: Either:
- Use this constant in the workflow by exporting it as an environment variable
- Remove this unused export to keep the code clean
If keeping it for documentation purposes, add a comment explaining why it's not used in the workflow.
| for (const {stageType, prefix} of stages) { | ||
| const env = {account: ACCOUNT_ID, region: REGION}; | ||
|
|
||
| const serviceStack = new ServiceStack(app, `${prefix}-ServiceStack`, { |
There was a problem hiding this comment.
LOW - Code Quality: Redundant stackName Property
The stackName property is redundant since it's identical to the construct ID (second parameter). CDK will use the construct ID as the stack name by default.
Current code:
new ServiceStack(app, `${prefix}-ServiceStack`, {
stackName: `${prefix}-ServiceStack`, // Redundant
env,
stageType,
});Recommendation: While explicit is fine for documentation and ensuring exact CloudFormation stack names match, consider adding a comment explaining this is intentional for the zero-diff takeover migration from CodePipeline:
// Explicit stackName ensures exact CloudFormation stack name match during migration
const serviceStack = new ServiceStack(app, `${prefix}-ServiceStack`, {
stackName: `${prefix}-ServiceStack`,
env,
stageType,
});
| - name: CDK synth | ||
| run: npx cdk synth --all --output build/cdk.out | ||
|
|
||
| - uses: actions/upload-artifact@v4 |
There was a problem hiding this comment.
LOW - Security: Artifact Retention
The uploaded artifact doesn't specify a retention period. GitHub defaults to 90 days, which may be longer than necessary for build artifacts that are only needed for the duration of the deployment.
Recommendation: Add explicit retention to reduce storage costs and limit exposure window:
- uses: actions/upload-artifact@v4
with:
name: cdk-out
path: build/cdk.out
retention-days: 1 # Only needed for this workflow run
| - beta | ||
| - prod | ||
|
|
||
| concurrency: |
There was a problem hiding this comment.
LOW - Operational Insight: Limited Concurrency Control Visibility
The cancel-in-progress: false queues deployments, which is good for safety, but there's no visibility into queued deployments or timeout configuration.
Recommendation: Consider adding:
- A timeout at the workflow level to prevent indefinite queuing
- A notification or comment when a deployment is queued
Example:
concurrency:
group: deploy-${{ github.ref }}
cancel-in-progress: false
jobs:
build:
timeout-minutes: 30 # Prevent indefinite runs
runs-on: ubuntu-latest1 participant
Summary
lib/app.tsto instantiate stacks directly with explicitstackNameproperties matching existing CloudFormation stack names (Beta-ServiceStack,Beta-MonitoringStack,Prod-ServiceStack,Prod-MonitoringStack) for zero-diff takeoveraarch64tox86_64for GitHub Actions runnersPre-merge checklist
CDK_DEPLOY_ROLE_ARNsecretbetaandprodcreated (prod with required reviewers)cdk diffagainst all 4 stacks to verify zero resource changesPost-merge
PipelineCloudFormation stacklib/stacks/pipeline.tsTest plan
npm run buildcompiles cleanlycdk listoutputs the 4 expected stack namescdk diffshows zero changes for all deployed stacks