Skip to content

Latest commit

 

History

History
108 lines (70 loc) · 7.35 KB

platform-acceptance-criteria.md

File metadata and controls

108 lines (70 loc) · 7.35 KB
Raw
title description layout redirect_from category subcategory
Acceptance Criteria for Platform PRs and Issues
How to create and manage pull requests and issues for the Login.gov Platform Teams.
article
Platform
Team

Overview

This document is used to define the minimal acceptance criteria (AC) for pull requests (PRs) (a.k.a. merge requests) and issues for the Login.gov Platform teams.

Style and Sanitizing

When creating PRs/issues, it's a good idea to follow these general guidelines:

  1. Replace ANY/ALL sensitive strings with <REDACTED> strings. This includes tokens, keys, etc.
  2. Contain all content within code blocks, using language syntax highlighting when possible
  3. Remove all unnecessary sections of content, i.e. excess log messages (replacing with links to CloudWatch logs, if possible)
  4. Use [...] to show gaps where extra content was removed between output you are including

Organizing Large Output with 'details' Dropdown Blocks

If the command-line output demonstrating the PR's changes is particularly large in size (i.e. 20+ lines), it can be wrapped within a <details> HTML block, which will output a clickable, dropdown text link when the description is submitted:

<details><summary> <tt>terraform plan</tt> output </summary>

 `` ``` ``

 `` ``` ``

 </details>
  • Text within the <tt> tags will be marked as code; these tags can be removed if desired
  • The line breaks between the <details> tags, and the actual code block, must remain in order for the code block formatting to render correctly.

Pull Requests

In order to be accepted for review, the description of a pull request should contain:

  1. A note of what is being changed/fixed by the PR.
  2. Output(s) demonstrating that the changes can be implemented without errors (more information below).
  3. Any corresponding issue(s) that this PR addresses (as, generally, a PR will only address part of an issue's AC)
  4. Information regarding any breaking changes that may/will result once the change is implemented, along with steps detailing how to mitigate/roll back these changes in case of an error
  • PRs of this nature should have the word 'BREAKING' in the title and/or use the BREAKING GitHub label
  1. Whether or not the changes require multiple steps (i.e. Terraform state operations, tfvars updates, etc.) to be fully integrated, before or after merging

Terraform

All PRs containing Terraform changes must include or point to the output of a terraform plan for each "state" type that will be changed when terraform apply is run. Said plan(s) should indicate the change(s) that will happen against 'production' (i.e. an existing environment up-to-date with a specific release and/or main), and that the plan operation executes successfully. As these outputs are generally very large, they should be organized in 'detail' blocks (as described above).

NOTE: A streamlined, easy-to-follow plan output can be generated by running bin/td -e <ACCT-OR-ENV> -d <DIRECTORY> -c b, which will invoke bin/tfplan, printing out a summarized module list along with the complete plan output.

Chef Cookbooks/Recipes

As much as possible, PRs containing changes to cookbooks (whether in identity-devops or other repos) should provide sufficient evidence showing that Chef executed them correctly. Depending upon the scope of the change (and/or desired outcome), this could include:

  • a section of /var/log/cloud-init-output.log showing the Chef operations completing successfully (with minimal surrounding context), or a link to said log in CloudWatch
  • the output of data/commands run on a host that was successfully Chef'd, such as cat-ing a file to show that changes to its contents were done successfully
  • an indication of which host type(s), within a specific environment, are currently running and contain the changes in the PR (so the reviewer can connect to a host and inspect for themselves)

Issues

Most of the style/formatting guidelines listed above for PRs can be similarly followed for Issues. As much as possible, use the in-repo templates (shown in the GitHub web interface, and/or the .github/ISSUE_TEMPLATE directory in the repo itself) when creating issues.

Detailed information on how to effectively write and define Issues can be found in the Definition Of Ready document in this handbook.

Appendix

Why do PRs matter?

Pull Requests (a.k.a. Merge Requests) are the magic tool that allows us to move quickly and safely through continuos change:

  • They are a way to lift each other up to higher standards, learn from each other, and improve
  • They protect against intentional or unintentional damage to the system
  • They are a critical part of how we meet the NIST 800-53 CM control family

For more on the art of the PR see The Art of Giving and Receiving Code Reviews (Gracefully).

What level of detail should a PR include?

It all depends on the scope of the work. Minor updates shouldn't need a lot of detail, whereas more drastic changes should include comprehensive information. In general, it can be additionally helpful to include any/all of the following:

  1. Which environment(s)/account(s) these changes have been verified to work in. Most of the time, this will be a personal sandbox environment, and can be tested there without issue. If the changes impact an account, such as those within Terraform directories core or all, it is acceptable to test, then roll back, the changes in the code within a sandbox/dev account, as long as the Platform team is notified (within Slack) when the testing is about to take place.

  2. Any breaking changes that will occur as a result of this PR being merged, whether just to main or when a particular stages/ branch is fast-forwarded to a new release. The author of the PR must inform any/all individuals whose work may be impacted by the change, and tag the PR with the BREAKING label. Most of the time, this will be the On Call engineer who deploys a release containing this PR's changes.

  3. Steps detailing, if they are necessary, any additional operations required in order to fully implement the change(s). These primarily will include changes to resources within the Terraform state, such as state rm/mv, import, etc. which must be performed manually before/after the plan is applied.

  4. Similarly -- if necessary -- steps detailing any mitigation steps, which fall outside of normal mitigation steps, that will need to be taken/recognized in case the plan needs to be rolled back. These will almost always need to be listed if there are any breaking changes, as defined in #2 above.