Add CI/CD Pipeline #1
Conversation
cdk/bin/api.ts
Outdated
| @@ -6,12 +6,16 @@ import { VpcStack } from "../lib/vpc-stack"; | |||
| import { RDSStack } from "../lib/rds-stack"; | |||
|
|
|||
| const app = new cdk.App(); | |||
| const rdsPasswordArnSsmParamName = "rds-password-secret-arn" | |||
There was a problem hiding this comment.
Ah nice that way we don't need to define it multiple times. Do you mind copying the arn from here https://github.com/austinloveless/awsmug-serverless-graphql-api/blob/master/cdk/lib/api-stack.ts#L22 and added it on line 9
There was a problem hiding this comment.
I'm not a big fan of the hard coded arn, also that doesnt work outside of your account
| @@ -0,0 +1,11 @@ | |||
| #!/usr/bin/env node | |||
There was a problem hiding this comment.
Why not add this stack as part of the root api.ts? I think naming is a bit confusing and I will update the file names to be a more vague umbrella, but I think the idea is to have everything specific for this API inside that bin/api.ts file that supports deploying and hosting that service. What are your thoughts
There was a problem hiding this comment.
This also uses a new logical "App()" instead of the existing one.
There was a problem hiding this comment.
I'm not running api.ts at all when deploying the pipeline stack
| @@ -1,5 +1,7 @@ | |||
| require("dotenv").config(); | |||
There was a problem hiding this comment.
If we're removing the ARN from the file and moving it to the root in bin/api.ts can you remove instances of require("dotenv").config(); and add that to bin/api.ts. It's no longer needed if we're not pulling env variables.
cdk/lib/rds-stack.ts
Outdated
|
|
||
| const secretArn = StringParameter.valueForStringParameter(this, props.rdsPwdSecretArnSsmParameterName); | ||
| this.rdsPassword = Secret.fromSecretAttributes(this, "rdsPassword", { | ||
| secretArn: secretArn |
There was a problem hiding this comment.
nitpicky but you can reduce this to just secretArn and remove the key.
There was a problem hiding this comment.
Yeah but that hard codes the arn again, which would have to be changed in every account it gets deployed in. Prob best to see if the partial arn search feature of CDK secrets actually works
cdk/lib/rds-stack.ts
Outdated
| @@ -38,7 +46,7 @@ export class RDSStack extends Stack { | |||
| vpc: props.vpc, | |||
| credentials: { | |||
| username: this.rdsDbUser, | |||
| password: this.secret.secretValue, | |||
| password: this.rdsPassword.secretValue, | |||
There was a problem hiding this comment.
does this need .toString() I saw in api-stack.ts it had .secretValue.toString() appended to it?
There was a problem hiding this comment.
Nay, the RDS construct takes an ISecret or something, according to the typing on the constructor for RDS. The Lambda constructor takes a string tho, feels like the two teams weren't on the same page.
| "@aws-cdk/core": "1.115.0", | ||
| "@aws-cdk/core": "^1.115.0", | ||
| "@aws-cdk/pipelines": "^1.115.0", | ||
| "apollo-server": "^3.1.2", |
There was a problem hiding this comment.
I think we can remove all the packages below "@aws-cdk/pipelines": "^1.115.0", I think those are only needed in api/
There was a problem hiding this comment.
oops keep source-map-support though.
There was a problem hiding this comment.
im guessing you had to add those when testing this to get the API to build with all npm packages? I think we can change this to be two pipelines one for API changes and one for cdk changes.
There was a problem hiding this comment.
I couldnt get it to build until I added those
There was a problem hiding this comment.
it was failing in codebuild FWIW
kevasync
force-pushed
the
master
branch
3 times, most recently
from
5844aca to
313c264
Compare
No description provided.