Skip to content

Contribution Procedure

Jump to bottom
ochan1 edited this page Mar 7, 2022 · 15 revisions

When you add code to the website (or any similar CompServ repo), you should follow a general procedure to make sure the contribution is orderly and reviewed.

Context: Around Fall 2019 and Spring 2020, the code contribution to hknweb was a mess. In particular, no one would review Pull Requests when they came in. Contributors assigned two to four reviewers, and none of them would review the pull request for weeks.

Before Starting Development

  1. Create a fork of the Github repository, if you haven't done so yet.

  2. Clone your forked repository locally to a computer, if you haven't done so yet. (If you haven't done these two steps, you can look at the Comprehensive Setup Guide for details steps on what to do)

  3. Rebase from compserv/master to your master branch. Push the updated tip to origin. (There should be no conflicts, otherwise, you may have to merge and consult with other officers to determine what you changed and the next steps, like Pull Request from your master)

  4. Create a local branch for your project for your feature. Do all of your development in this branch, pushing to your forked origin repo as needed.

  5. Try to keep one kind of feature development in different Branches. If you need to mix features together, use your discretion or complete one Pull Request for one feature before creating another one.

Merging into Compserv

  1. Rebase or Merge your origin's master with compserv/master if there are changes since you started your project Example git commands:

    git remote add upstream <COMPSERV-REPO-URL>  # Do this line if you haven't yet added a CompServ repo upstream (with the ".git" in the end)

git fetch upstream                           # Fetch from "upstream", or CompServ repo
git checkout master                          # go to your local "origin" master branch

git rebase upstream/master

    It is HIGHLY recommended you rebase rather than merge your fork's master. If you have to merge or make changes to your rebase, that means you made changes to your master branch.

    If you have to make changes after rebase, attempt to fix the merges. If you need to, you can then consider merge by git merge upstream/master. Then make a new branch to keep the changes, having that branch point to what master is at this point, and then use git branch -f master <COMPSERV-MASTER-COMMIT-ID> and git push -f to 1) transfer your changes and edits onto another branch, and 2) keep your master up to date and consistent with compserv/master.

  2. Rebase or Merge your origin's master to your project branch if your branch is out of date, and fix any merge conflicts. Example git commands are:

    git checkout <BRANCH>      # go to your BRANCH

git rebase master          # rebase / merge your current branch (HEAD on BRANCH) with master
# OR
git merge master

  3. On the Compserv repository, create a new Pull Request from your fork's branch to compserv/master. Title it with your fixes and/or creations. Link any related issues.

  4. Assign one to two officers and/or assistant officers in CompServ on GitHub using the "Assign" feature, or other committee members if applicable, to review the PR. If the PR is urgent, message them on Facebook or Slack, or use your discretion to merge in immediately. Continue pinging in a few days if needed.

  5. Refer to Reviewing a Pull Request to start reviewing Pull Request

  6. After checks finish running, review all errors and issues, and fix all issues.

  7. If reviewers have any comments, address all comments by either fixing the issues or explaining why you think no changes are needed. Ask at least one reviewer to review your changes and comments. Repeat until approved.

  8. Merge the pull request.

Reviewing a Pull Request

  1. Every week, 1-3 people will be assigned to review Pull Requests for that week. Please let an officer know through Slack if you cannot review it within the week and we will assign someone else to do it in your place.

  2. If the PR is urgent, review as soon as possible (use your judgement). Otherwise, review it within a week. A good time to go over PR reviews is after the weekly CompServ meetings. Look over the commits and files changed, and pull from the contributor's branch to test the code locally. Make comments asking for clarifications or justifications, and if warranted, request changes to be made or approve the pull request. Remember to publish your review.

  3. When the contributor makes comments or changes, look over the changes. Make sure all of your points were satisfactorily addressed. Provide further comments or approve the PR.

Details Regarding Reviewing a Pull Request

(content from https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/checking-out-pull-requests-locally)

  1. To test the functionality of the Pull Request, it is best to run the requester's code onto your computer. There are two ways to do this, with (i) being recommended and easiest:

    1. Clone the Requester's Repo and Get the Pull Request's Branch. You will first git clone <REQUESTER_URL> and git checkout <BRANCH>. You can see which branch is used on the top of the Pull Request: "<REQUESTER> wants to merge 8 commits into compserv:master from <BRANCH>".
    2. To do this, navigate to the pull request, scroll down to the big button that says Merge Pull Request, and click command line instructions to the right of it. Follow these instructions to make a copy of the PR onto your computer.

  2. Although you have the option to make edits and merge your changes, we typically do not do this. Instead, view Files Changed, where you can see the changes in the code itself and add your review directly referencing pieces of the code itself. Doing this greatly helps the requester understand exactly what in their code needs to be changed.

    • You can also make the changes yourself and send them to the reviewer via Slack to review so they can push it.

  3. Additionally, for more general comments about the functionality of the code, you can go back to the Conversations tab and scroll all the way to the bottom of the page to leave a comment. These kinds of comments are warranted if something isn't working when you run the code, you have a general suggestion to make the PR better, you notice a frontend discrepancy with the rest of the website, etc.

Running Checklist to Look for During a Pull Request Review

  • Are certain functions and views given the correct permissions? (We want to avoid something like this with Mass Checkoffs)
  • Would this be reasonably easy to read for future developers?
  • Can the original author defend their use of certain design choices, up to a reasonable MVP standard if necessary?
Clone this wiki locally