Page Comparison

Ideally the description in the Jira task is written in a way that it can be copied and pasted as the description of the Pull Request. This is very subjective, but the description should:

• Clearly describe the change that is needed

• Be written in plain language - if it’s very technical then perhaps add a “Background” introduction section that explains things in plain language

• Contain an “Acceptance Criteria” section detailing what is needed for the task to be considered done

This is extremely important because you don’t want to spend time on a Pull Request only to find out that it has to be completely redone because of its core design. Also, there will always be improvements underway in the CHEFS code, and any new work must align with those improvements - so following the existing design/code might not be the best approach. The end goal of these improvements is code that is:

• necessary (YAGNI - don’t maintain, document, test, etc, code that we don’t need)

• correct (it should work!)

• simple (“as simple as possible but no simpler”)

• documented

• easy to read

• easy to understand

• easy to modify

• easy to fully test

• consistent (to reduce cognitive load)

Information that everyone should be familiar with:

• Contributing Guidelines

• CHEFS Technical Documentation

• OpenAPI Documentation for the backend

• Backend Design Document

• Backend Test Strategy

It’s only a recommendation that “working” commit messages use this format - these commit messages will eventually be squashed, and it’s only the final commit message that must be in this format. It’s a good idea to always use this format to be familiar with how the final commit message must look.

Tip: In the VS Code commit message input box, the up arrow allows you to scroll back through previous commit messages that you used.

We use the format type: FORMS-NNNN description for our Pull Request titles so that:

• the type of change is obvious from the name of the Pull Request

• it is easy to find all Pull Requests for a given type like feat or test

• it is easy to find the Pull Request for a given FORMS-NNNN Jira task

• the description makes it easy to scan Pull Requests to find a change that happened

Please make the description meaningful:

• BAD: feat: FORMS-1234 add feature

• GOOD: feat: FORMS-1234 add map component

• BAD: fix: FORMS-2345 fix bug

• GOOD: fix: FORMS-2345 fix layout bug with Data Grid

• BAD: test: FORMS-3456 add tests

• GOOD: test: FORMS-3456 finish test coverage for email service

If your changes won’t fit in the a short description, it’s possible that your Pull Request is probably doing changing too many things.

This is a chance to do a self-review of your changes before creating the PR

• are the changes too big (too many files, too many lines) to be easily and quickly reviewed?

• is the Pull Request doing too many different things? Can it be broken down into smaller PRs?

• did reading the code remind you of work you forgot to do?

• did you leave yourself ‘todo’ items that need to be completed?

• is every changed line necessary? Was anything accidentally committed?

• are there tests to cover the code changes / additions?

• are there documentation updates for the code changes / additions?

The “Tests” GitHub Action runs:

• GitHub CodeQL tests to look for security issues

• Backend Jest unit tests, including and saves test coverage saved as an artifact

• Frontend Jest unit tests, including and saves test coverage saved as an artifact

Note that the test coverage is not uploaded to Code Climate, as Actions run from Pull Requests do not have access to the GitHub Secrets needed to authenticate with Code Climate.

There are various ways to tell this:

• Watching the Action you will see the Deploy step complete and display the URL of the deployment

• Unless you have the notifications notification disabled, you should receive an email

• The Action will add a comment to the Pull Request

Ideally every developer would review and understand every change in every Pull Request, but that’s not practical. Code reviews have many purposes:

• Sharing knowledge: more people understanding the changes means better group team knowledge of the code

• Enforcing consistency: the code should look like a team effort, not a dozen people doing things their own way - this reduces cognitive load when working with the code

• Preventing bugs: the goal of a review is not to test that someone’s code works, that’s the developer’s job. Reviewers very familiar with the code being changed, though, are likely to catch bugs introduced by people less familiar with the code - this is a great learning opportunity.

There are two ways of looking at Pull Requests:

1. Those that use existing concepts / designs vs. those that introduce new concepts / designs

2. Those that are simple changes vs. those that are complex changes

We can use the Ship / Show / Ask approach to Pull Requests:

• Existing concepts / designs + simple changes: Ship it (example: dependency update, typo fix, small bug). Show or Ask for a review if you are uncertain or otherwise feel it’s necessary

• Existing concepts / designs + complex changes: Show / tell other developers about the changes but doesn’t necessarily need a review (example: new feature that’s very similar to existing features). Ask for a review if you are uncertain or otherwise feel it’s necessary

• New concepts / designs + simple changes: Ask for a review (example: small bugfix that needs a big new way of doing things)

• New concepts / designs + complex changes: Ask for a review (example: new feature that’s different from anything we have)

This step really depends on the reviewer. Some people notice right away that they have been asked to do a review, but many do notthat isn’t guaranteed. It doesn’t hurt to post in the team channel on Discord that something is ready for review and “mention” the reviewers.

If your change is complex or if your team members are less familiar with that part of the code, it might not hurt be good to set up a meeting and to explain the changes

It depends! It’s good to always be up to date because the testing and review is more meaningful if the base branch is up to date. However, it can also waste a lot of time if one person is putting in multiple small changes and everyone is you’re trying to keep upupdate for every merge. Find a balance between having a current base and spending time keeping it current.

You can use the GitHub web site to update your branch, but note that this leaves the branch in your local environment out of sync. This might be OK if the update is for completely different parts of the code, but if there are conflicts then its it's better to update locally and push to your branch.

If you or others are testing the deployed code, but the code is not up to date with main, then bugs could sneak through. When you look at your PR Pull Request there will be comments in the timeline for commits, and comments from the PR Deploy Action. If there are commits after the last PR Deploy, then it should be deployed again. Retesting might be wanted if there were merge conflicts or other contention between the new main and the PR Pull Request code.

feat: FORMS-1234 new map component for dropping a pin
 
Added a new map component that allows the user to drop a "pin" on the map,
and the location is saved as botheither Lat/Lon or UTM coordinates.

This format must be followed as we eventually want to introduce automated changelogs and versioning.

The Push Action runs on merge, and:

1. Build Builds the application

2. Does a no-outage deployment to dev

3. Waits for gatekeeper approval to deploy to test

4. Does a no-outage deployment to test

5. Waits for gatekeeper approval to deploy to prod

6. Does a no-outage deployment to prod

Once you have merged your changes, ensure that they are deployed through all environments. Do not leave a deployment “hanging” and deployed to only the lower environments, as the next merge will probably cause it to go all the way to prod. If the change is tested and approved, it should go to prod as soon as possible.

Maybe? Merge can only be done by people with write access to the repo, so in theory on merge we would have access to the GitHub secrets and could undeploy automatically if we can figure out the PR number. If this doesn’t work then the “dumb” way of doing it would be to figure out with PRs are deployed and then see if they are open or not.