Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add docs on setting up a new project #340
Add docs on setting up a new project #340
Changes from 1 commit
23758c5
9e95c50
24f4920
7f5eccb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Are you sure you want to allow self-merge when the PR has the potential to destroy things? (the second point you wrote above)
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.
The danger is in the
terraform apply
step, which is why it doesn't run automatically. Merging doesn't actually trigger anything.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.
OK, thanks for the clarification, so how do we make sure others can see/review that "state" before being applied?
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.
Great question! What kinda workflow do you think would work here? I don't want to run
terraform apply
in CI, so not sure how to approach review.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.
I have not worked with the terraform stuff yet (although I have some experience with AWS Cloudformation), but looking quickly at the terraform docs, it seems you have a
terraform plan
command. That should surface any problem (or deletion you are trying to prevent), right? Then, you could run that command by the CI and detect any "bad" message. And others can review that plan.Additionally, it seems you can run that command with a
-out
parameter and get some file that you can eventually run manually later withterraform apply
command (modulo you still do not want to runapply
within the CI which could be an event happening on the merge event after a successful CI run and approval from others, but I totally get it is frightening to give the CI such destructive power): https://www.terraform.io/docs/cli/commands/plan.htmlFinally, I may be saying stupids things (as I did in the other PR) because I do not know yet the deep stuff, so be patient with me 😉
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.
I'm a little bit confused now as step 3 is "Check the project has been created and you have access to it" and step 4 is "open the PR" which implies
terraform apply
was run before opening the PR?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.
Ah, my brain just caught up with me! Yes, I think we should approve/merge the PR before running
terraform apply
- always good to have a second pair of eyes on code/configs before actioning anything I think.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 is where
terrraform plan
(withsensitive
on to prevent leaking info/secret) plays a role, IMHO.Otherwise, the person reviewing the PR does not have any info about how the plan looks like.
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.
@damianavila @sgibson91 I updated the instructions based on the conversations here. LMK what you think? I wanna balance code review with appropriate load on our small team.
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.
I think this looks good!