Teh Guide

This page is read only. You can view the source, but not change it. Ask your administrator if you think this is wrong.
This post is about reviewing Pull Requests (PRs) on a software engineering team, often submitted in a source code system like Github. It’s meant to be a **concise** guide.

===== Goals of a PR Review =====

  * Catch issues in the code that the submitter may have missed
  * Offer advice in making the code submission better
  * Complete any team-specific PR review required steps

For many PRs, you don’t need to be an expert in the stack or programming language that is used in the PR. 

The time it takes to review a PR is directly related to how long the PR took to create and how many changes it includes. You should expect to spend more time reviewing a large PR than a small PR.

===== Required Steps When Reviewing =====

The steps below apply to PRs with changes to code. If it’s a non-code PR (e.g. a wiki update), the reviewer should just check for typos and that the changes make sense to them.

  - Confirm that the PR description contains at least 1-2 items describing the changes being made and the reason for those changes.
  - If there aren’t automated tests, confirm that the PR description contains at least 1-2 items describing how the changes were tested.
  - Check each changed file for potential bugs that the submitter may have missed.
  - Check to see if [[https://tehguide.com/readable-code|__the code is readable__]].
  - Check for typos in the comments and readmes.
  - If you’re not proficient with the programming language or framework used, make a note of that in your review.
    - For example, "I'm a beginner when it comes to JavaScript. This PR looks good to me, but if you have any doubts about your code or would like a more in-depth review, please consider requesting a second review in %%__%%_ channel."
    - You don't have to be an expert in the system which the PR is changing, but you should have a basic understanding of what's going on. If you have absolutely no clue, it's best to either loop in someone who does, suggest that the submitter bump their request for a review, or ask for a review in another channel. 

===== Recommended Steps When Reviewing =====

  - If there are any parts that are particularly difficult to understand, consider suggesting that the submitter refactor or add a comment.

===== Example =====

🎉 Lgtm!

My gitlab CI configuration experience level: beginner.

I did not see any obvious errors, everything looks readable and elegant.

If at all possible, try to make future PRs smaller. If not possible, please note that in the PR description.

  * If many files are being renamed, I suggest making a separate PR just for the renaming.
  * There were also formatting/lint updates in this PR. In the future, I suggest a separate PR or including automatic changes like that in a separate renaming PR.

ci/scripts/pipelines.py

  * Is it possible to move the configuration constants (e.g. ROLE_PREFIX) to a config file or AWS systems manager? If yes, no need to change this PR, but please consider for a future PR.

It looks like there are some major changes to the gitlab pipeline in this PR. I reviewed the testing methodology, and it looks pretty good, but due to the size of this PR, please take a second look at the tests you did to make sure these changes wont break anything before merging the PR.

Great work 🔥. Thank you for making these gitlab CI improvements!