-
Notifications
You must be signed in to change notification settings - Fork 3
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
Create Staging and Production environments; Refactor and create necessary modules and resources #67
Create Staging and Production environments; Refactor and create necessary modules and resources #67
Conversation
Remove `README.md` because it outlines a manual process that is not automated by terraform/opentofu
The IAM roles might need to be modified before this PR is merged
ami = data.aws_ami.ubuntu.id | ||
instance_type = var.instance_type | ||
subnet_id = data.terraform_remote_state.shared.outputs.subnet_id | ||
# key_name = var.ec2_key_name |
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.
We might want separate keys for stage and prod
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.
We discussed this on a call and we're going to be using a pre-existing key for both instances
082a2eb
to
16ca672
Compare
16ca672
to
591dafa
Compare
"Effect": "Allow", | ||
"Action": [ | ||
"ec2:*", | ||
"ecr:*", | ||
"s3:*", | ||
"dynamodb:*" | ||
], | ||
"Resource": ${resources} | ||
}, | ||
{ | ||
"Effect": "Allow", | ||
"Action": "ecr:GetAuthorizationToken", | ||
"Resource": "*" |
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.
These are too permissive, and we will refine them in the very near future
output "public_ip" { | ||
value = aws_eip.staging.public_ip | ||
module "ec2" { | ||
source = "../modules/ec2/" |
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.
This will need to be changed to a github URL with a reference to a tag after this PR is merged
} | ||
|
||
module "ec2" { | ||
source = "../modules/ec2/" |
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.
This will need to be changed to a github URL with a reference to a tag after this PR is merged
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.
@smokestacklightnin why not leave it like this? What's the advantage of referencing the source as a GitHub URL?
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.
If the ec2 config changes from stage to prod, this is how we can keep track of those changes
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.
You mean the module changing? I'd say let's not overcomplicate things for now and keep the reference to the local module
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.
Thanks. LGTM, but let's discuss with @agt24 and @marcelovilla before merging.
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.
@smokestacklightnin this looks good but we need to make some minor adjustements before merging it:
- Uncomment elastic IP resource and output blocks
- Use pre-existing key pair for both instances
I suggest we merge the PR after having deployed the infrastructure on the NIH AWS account
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.
@leej3 I'm approving this PR but I'd like for you to take a look at the non-Terraform changes that we're suggesting
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.
Thanks. Looks good to me. I can debug any remaining issues in the python code post merge. I'll most likely do that as part of my WIP PR.
Can you add an explanation for the stored state files (perhaps compared to just storing them ourselves somewhere.
This PR addresses Issues #60, #61, #62, and #63.
The following changes are made:
This PR should not break the current deployment because the deploy workflow is only run on tagged pushes.