-
Notifications
You must be signed in to change notification settings - Fork 0
Env service refactor #58
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.
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
base: master
Are you sure you want to change the base?
Conversation
… stack missing error
Custom packages
implement remove for custom providers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR upgrades dependencies (AWS CDK, aws-sdk, js-yaml, and others) and refactors the deployment provider system to support multiple Serverless Framework versions (v1, v2, v3) and AWS CDK. It also adds support for environment variable-based configuration, SNS notifications for deployment status, and secret resolution from SSM.
- Updates dependencies including AWS CDK (1.94.1), aws-sdk (2.869.0), js-yaml (4.0.0), and adds new packages (chalk, flat, ncjsm)
- Refactors Serverless provider to handle v1, v2, and v3 compatibility with dynamic module resolution
- Adds SNS client for publishing deployment results to Frankenstack
- Implements secret resolution from AWS Systems Manager Parameter Store
- Renames class from
ServerlessV1toServerlessand method fromdestroytoremove
Reviewed Changes
Copilot reviewed 13 out of 16 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Updates dependencies and removes serverless package from direct dependencies |
| lib/utils/parseYaml.ts/js | Updates js-yaml API from deprecated safeLoad to load |
| lib/service/snsClient.ts/js | New service for publishing deployment status messages to SNS |
| lib/providers/serverless.ts/js | Major refactor to support multiple Serverless Framework versions with credential handling |
| lib/providers/hardcoded.ts/js | Updates property names and adds result field to response |
| lib/providers/cdk.ts/js | Adds remove method, credential handling, and refactors deploy logic |
| lib/deployer.ts/js | Adds environment variable parsing, secret resolution, and SNS integration |
| bin/index.ts/js | Makes file parameter optional and adds default command |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| console.log('resolved config', configuration, util.inspect(variablesMeta, {showHidden: false, depth: null, colors: true})); | ||
| } catch(err) { | ||
| console.error('Error while resoving serverless template.', err); |
Copilot
AI
Nov 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected spelling of 'resoving' to 'resolving'.
| console.error('Error while resoving serverless template.', err); | |
| console.error('Error while resolving serverless template.', err); |
| } else { | ||
| const result = {status: false} | ||
| await this.publishResultToFrankenstack(result); | ||
| throw `Failed to resolve secret value for paramater: ${paramName}`; |
Copilot
AI
Nov 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected spelling of 'paramater' to 'parameter'.
| throw `Failed to resolve secret value for paramater: ${paramName}`; | |
| throw `Failed to resolve secret value for parameter: ${paramName}`; |
| "@aws-cdk/core": "^1.94.1", | ||
| "aws-cdk": "1.94.1", | ||
| "aws-sdk": "^2.869.0", | ||
| "chalk": "^5.0.0", |
Copilot
AI
Nov 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chalk v5.0.0+ is ESM-only and incompatible with CommonJS require() statements used throughout this codebase. Since chalk is not currently imported anywhere, this unused dependency should either be removed or downgraded to v4.x which supports CommonJS.
| "chalk": "^5.0.0", |
| const client = new SNS(); | ||
|
|
||
| module.exports = { | ||
| async publishJobRunFinishedMessage(jobRunFinishedResult: any) { |
Copilot
AI
Nov 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing validation for process.env.JOB_RUN_FINISHED_TOPIC_ARN. If this environment variable is undefined, the SNS publish call will fail. Add validation to ensure the TopicArn is defined before attempting to publish.
| async publishJobRunFinishedMessage(jobRunFinishedResult: any) { | |
| async publishJobRunFinishedMessage(jobRunFinishedResult: any) { | |
| if (!process.env.JOB_RUN_FINISHED_TOPIC_ARN) { | |
| throw new Error('Environment variable JOB_RUN_FINISHED_TOPIC_ARN is not defined.'); | |
| } |
| return; | ||
| } | ||
| for(var key of Object.keys(this.component.inputs)) { | ||
| if(this.component.inputs[key].startsWith('ssm:')) { |
Copilot
AI
Nov 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential TypeError if this.component.inputs[key] is not a string. The code should verify that the value is a string before calling startsWith().
| if(this.component.inputs[key].startsWith('ssm:')) { | |
| if(typeof this.component.inputs[key] === 'string' && this.component.inputs[key].startsWith('ssm:')) { |
| options['stage'] = this.stage; | ||
|
|
||
| if(this.account) { | ||
| options['aws-profile'] = 'frank' |
Copilot
AI
Nov 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid automated semicolon insertion (90% of all statements in the enclosing function have an explicit semicolon).
| options['aws-profile'] = 'frank' | |
| options['aws-profile'] = 'frank'; |
| } | ||
|
|
||
| if(this.account) { | ||
| process.argv.push('--aws-profile', 'frank') |
Copilot
AI
Nov 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid automated semicolon insertion (90% of all statements in the enclosing function have an explicit semicolon).
| process.argv.push('--aws-profile', 'frank') | |
| process.argv.push('--aws-profile', 'frank'); |
| await sls.init(); | ||
| } | ||
| try { | ||
| console.log("Executing serverless run.") |
Copilot
AI
Nov 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid automated semicolon insertion (90% of all statements in the enclosing function have an explicit semicolon).
| console.log("Executing serverless run.") | |
| console.log("Executing serverless run."); |
| await sls.run(); | ||
| } catch(err) { | ||
| console.error(err); | ||
| success = false |
Copilot
AI
Nov 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid automated semicolon insertion (90% of all statements in the enclosing function have an explicit semicolon).
| success = false | |
| success = false; |
| return { | ||
| result: success, | ||
| outputs: outputs | ||
| } |
Copilot
AI
Nov 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid automated semicolon insertion (90% of all statements in the enclosing function have an explicit semicolon).
| } | |
| }; |
No description provided.