Conversation
infra/cdk/test/cdk.test.ts
Outdated
| test('SQS Queue Created', () => { | ||
| // const app = new cdk.App(); | ||
| // // WHEN | ||
| // const stack = new Cdk.CdkStack(app, 'MyTestStack'); |
There was a problem hiding this comment.
@iamusmanazeem can you write test cases for our current stack
There was a problem hiding this comment.
I can't stress this enough that we really need to follow the practice of TDD
infra/cdk/lib/cdk-stack.ts
Outdated
|
|
||
| securityGroup.addIngressRule( | ||
| ec2.Peer.anyIpv4(), | ||
| ec2.Port.tcp(80), |
There was a problem hiding this comment.
I would encapsulate this entire logic in a separate class you can name it EC2
There was a problem hiding this comment.
And tests is a must have
infra/cdk/lib/cdk-stack.ts
Outdated
|
|
||
| // cdk lets us output prperties of the resources we create after they are created | ||
| // we want the ip address of this new instance so we can ssh into it later | ||
| // new cdk.CfnOutput(this, 'simple-instance-1-output', { |
There was a problem hiding this comment.
i believe we no longer need this comment
There was a problem hiding this comment.
yeah, we might need this syntax in the future. we can remove it now
package.json
Outdated
| "start:dev": "nx start:dev api", | ||
| "test": "nx run-many --target=test --all" | ||
| "test": "nx run-many --target=test --all", | ||
| "start:deploy": "nx run-many --target=deploy --all" |
There was a problem hiding this comment.
just add deploy. start:deploy doesn't make sense
pnpm-lock.yaml
Outdated
| @@ -1,8 +1,4 @@ | |||
| lockfileVersion: '6.1' | |||
There was a problem hiding this comment.
@Shazil2154 add this line in .gitignore
pnpm-lock.yaml
**/pnpm-lock.yaml
then delete this pnpm-lock.yaml file and push
There was a problem hiding this comment.
since we all are using different versions of pnpm this will cause issues. That is my bad but let's fix it in this pr
| "watch": "tsc -w", | ||
| "test": "jest", | ||
| "cdk": "cdk", | ||
| "deploy":"cdk deploy --profile roadoxe" |
There was a problem hiding this comment.
was adding a profile in the command a good idea? Because we might name our profiles differently
There was a problem hiding this comment.
I guess yeah it should be just cdk deploy
|
@iamusmanazeem this pr was a draft if you need a review on a pr or want to merge it. It needs to be open |
|
The changes should've been merged in the first merge commit why do we have to add the commits twice? Each different part should be an isolated commit. I don't see a commit of you pushing the CDK code which means you batched it with the merge commit which is not a good approach. We really need to pick up best practices we are just wasting our time with this project. If we are gonna do the same things we have done up until now what's the point of working on this project? |
3 participants
No description provided.