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

Add code review guidelines for changing running infrastructure #516

Merged
merged 5 commits into from
Sep 2, 2021

Conversation

yuvipanda
Copy link
Member

@yuvipanda yuvipanda commented Jul 14, 2021

Writing and reviewing terraform code has different challenges than
reviewing other kinds of code. This is a draft attempt at
codifying how code review and merges work with terraform in our
repo, so we can make changes safely and quickly

closes 2i2c-org/team-compass#213

Writing and reviewing terraform code has different challenges than
reviewing other kinds of code. This is a draft attempt at
codifying how code review and merges work with terraform in our
repo, so we can make changes safely and quickly
Copy link
Member

@sgibson91 sgibson91 left a comment

Choose a reason for hiding this comment

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

As a draft, I think this is a great start. Clearly explains why the code review is complex and how the different scenarios of code review differ with clear process for each 👍🏻 Thank you!

Copy link
Contributor

@damianavila damianavila left a comment

Choose a reason for hiding this comment

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

I left a question but nothing blocking this PR.
Even as a draft it is a great first step and I generally agree with the described workflows!

Copy link
Member

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

This is helpful context on Terraform + I like the process around it. Thanks for this write-up!

I have two quick comments but they're both related to the same idea around "state of cluster" vs. "state of codebase". Curious what others think, but I don't think either should block this PR

docs/contributing/code-review.md Outdated Show resolved Hide resolved
docs/contributing/code-review.md Outdated Show resolved Hide resolved
@choldgraf choldgraf changed the title Add code review guidelines for Terraform code Add code review guidelines for changing running infrastructure Aug 25, 2021
@choldgraf
Copy link
Member

I've just pushed some changes to this PR after some of our recent conversations and feedback! Would love another round of reviews from people to see if this looks good to folks!

@choldgraf
Copy link
Member

I requested reviews from some more team members, since my push changes and updates the wording a little bit. It would be good if at least one person could give this a 👍 and make sure that we haven't missed anything important.

Also I can't request a review from @yuvipanda since he's the author, but I would love if he could confirm that this PR still looks good to him w/ my changes :-)

Copy link
Member

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

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

I made two suggestions, but otherwise this LGTM! Nice work!

docs/contributing/code-review.md Outdated Show resolved Hide resolved
docs/contributing/code-review.md Outdated Show resolved Hide resolved
@choldgraf
Copy link
Member

many thanks for the review @consideRatio - changes accepted!

for others - let's leave this open for another 48 hours. If somebody else approves then I think we can just merge it in unless somebody objects.

Copy link
Contributor

@damianavila damianavila left a comment

Choose a reason for hiding this comment

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

LGTM

@damianavila
Copy link
Contributor

LGTM

Not sure about the CI failure but content LGTM...

@choldgraf
Copy link
Member

OK - we've got a few approves, and CI is now happy, and we've given it several days, so I'm gonna merge this one in! Thanks for all the feedback everybody :-)

@choldgraf choldgraf merged commit 748d951 into 2i2c-org:master Sep 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Policy to guide merges in pilot-hubs repo
5 participants