Topics on this page

Pull Request Code Review and Continuous Integration

BriteCore’s Solution Architecture team outlined the Pull Request (PR) below for all partner and client PRs targeted towards the master branch in the BriteCore repository.

The goal is to provide partners developers, client developers, and other third-party developers with timely responses on their PRs with minimal disruption to the Product teams’ normal workflows.

Partner PR Process flow

Step 1: Pre-Development

There is an approval process in place prior to the developer starting to code the feature. This ensures that the work is a safe, welcome change to BriteCore that adds value for multiple clients.

Step 2: Automated Reviews

Like all PRs targeting the BriteCore master branch, partner PRs need to pass the automated checks for linting, automated tests, and test coverage.

In some circumstances, a Solution Architect may choose to move this PR forward without one of these cases being met and will document their reasoning. Examples of this might include a test coverage report going down for an area of code that we’re comfortable not being fully covered, or allowing tests to be added in a follow-up PR for a critical issue.

Step 3: Solution Architecture Review

3.1: Adherence to the Pre-Development Solution

The Solution Architect ensures that the developer followed the agreed-upon predevelopment plan.

3.2: Code Cleanliness

The Solution Architect ensures the code is clean and easy to read. This includes formatting, variable naming, and comment/docstring cleanup.

3.3: Code Quality

The Solution Architect ensures the code is maintainable and of good quality. During this phase, requests for refactoring and increased test coverage are made.

3.4: Notice to Code Reviewers

If everything passes the Solution Architects’ review, they attach the following notice to the PR:

Attention Code Reviewers: The Solution Architecture team has reviewed this PR and found that it complies with BriteCore’s code quality standards. Furthermore, the functionality in this PR has been deemed valuable to one or more clients.

Any further review should be with the sole purpose of determining the following:

  1. The PR is safe.
  2. The PR does not block current initiatives from being implemented.
  3. The PR does not take away from the product in any way.

If any of the above are true, please leave a full explanation. Otherwise, please leave a passing review. If there are changes that you’d still like to see, please make a request for those to be addressed in a follow-up pull request.

Step 4: Product Review and QA

4.1: Code Reviewers

Product teams reviews should only focus on the following areas:

4.1.1: The PR is Safe

The Product teams know the functionality of their products the best. They make sure that any change being proposed is safe and operates as intended.

4.1.2: The PR doesn’t block current work

The Product teams know their backlogs best. They ensure that PRs don’t clash with current PRs and initiatives.

4.1.3: The PR doesn’t limit the product

The PR shouldn’t take away from BriteCore in any way or limit the ability to execute on future goals. Community PRs should benefit the community.

However, it’s entirely probable that during the course of their review, a product code reviewer might encounter changes that fall outside these three categories that they want to see changed. In that case, the product code reviewer should fully explain the changes they want, why they are needed and request that a change be made in a Follow-Up PR. This ensures the partner PR can move forward and the product’s quality concerns are both addressed.

Note: No additional code cleanliness-style requests usually take place in the current PR. However, follow-ups can be and are used.

4.2: Peer reviews

Although peer reviews are an important part of the overall review process and are an important learning tool, Peer reviews don’t take place on partner PRs.

4.3: QA

The PR should still be manually tested by the responsible parties to make sure it works as intended. Automated tests should be included with all PRs.

4.4: Solution Architect

Throughout this stage, the Solution Architect acts as the PR’s internal advocate to make sure that teams are reviewing and testing the PR in a timely manner.

Step 5: Merge

Once a PR has been approved by our automated checks, the Solution Architecture team, the Product teams, and QA, it will be merged into the master branch.

Step 6: Follow-Up

A Follow-Up PR is meant for code cleanliness issues the Product team would like to see changed. These are mostly aesthetic changes, as any performance or safety-related issues are addressed before the PR is merged.

If a request for a Follow-Up PR was created, the partner developer should open this new PR, and this process will repeat.

Note: Partner Developers who fail to create follow-up PRs endanger getting future PRs merged.

Exceptions

Although most partner PRs follow the process outlined above, there are exceptions:

Critical Issues

Although it’s rare that a partner will open a critical PR, if someone does, it will be shepherded through the process by the team helping with the critical issue.

PRs targeted towards other branches (not master)

Other branches have custom workflows and this process doesn’t apply.

Continuous Integration

Repositories that contain deliverables to production automatically enforce:

  • Passing tests.
  • Code coverage.
  • Code formatting.
  • Code linting.

Continuous Deployment

Repositories that contain deliverables to production are programmatically deployed.

  • Services (BriteAuth, BriteLines, etc.) are deployed to the AWS Service Catalog.
  • Packages (django-britecore, britecore/bc-design-system, etc.) are deployed to their respective package repositories.