Skip to content
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

Aws cdk #1

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Aws cdk #1

wants to merge 10 commits into from

Conversation

arjun123970
Copy link

Edited the original code that was given in the tutorial to fit into my requirements. I created a VPC, Security Group, Role, and an T2 Micro EC2 instance. Once the code has been somewhat finalized, I plan to edit the instance size to fit the source graph requirements.

@abosnjakovic
Copy link

Hey @arjun123970 I can see that you created CdkStarterStack 3 times, is there a reason why the code is replicated like this. even the .test file which is meant to test you code has been over written with the same implementation. the bin/ was meant to create the instance for deployment for example and doesn't really need the CdkStarterStack there.

Copy link

@abosnjakovic abosnjakovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please review comments

@@ -1,731 +1,73 @@
import {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can see this file is aiming to test the implementation, yet it was changed to something completely different. you should look at this diff hunk and ponder over it existence.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when you synth it should generate a terraform file that you could test against

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

restore this file and have a go at adding a test to validate an expected result

@@ -1,21 +1,73 @@
#!/usr/bin/env node

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can see in this diff hunk, the comments below describing some important aspects of this file. like the ability to change envs. really useful. please read it, ask questions and be curious about it. overwriting it with the same code from another file is not really adding value to what the point of this function aims for

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should learn to revert this change with git command line. look into git revert and hunks

@arjun123970
Copy link
Author

Hi Mr. Adam, I have pushed my new code to aws-cdk branch. I was able to get the npm run local synth command running and it gave me back a cloud formation template. I am, however, having trouble with deploying my stack. It thinks that I am trying to deploy to aws instead of localstack. I was also looking over the tutorial and the author stated that they already had the localstack api key. I was wondering how I should set it in my environmental variable as it is currently saved as an empty text string.

…m the yml. file. I was also able to get local stack working but I do not have permission to see the website in my web browser. I am currently working on that right now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants