How to Process PRs

General Mindset

• Think breadth-first, not depth first. Open a bunch of tabs and start as many conversations as necessary. Most PRs have at least one feedback cycle - it's best to have as many in flight as possible so we don't keep both sides waiting.

• Be appreciative. Even if the PR has problems, they took the time to submit it. Thank them!

• Comment early and comment often. No one likes waiting for their PR to be acknowledged. Level with them - ask questions, don't be a roman emperor. Work with them towards the end goal if necessary, and give guidance if their initial approach isn't quite right. Let them know about any architectural concerns or long-term vision that may affect the approach, no matter how big the scope may be.

  Don't expect them to be willing to take on a much larger amount of scope, but give them the opportunity, while letting them know it's OK to not have the time/confidence to follow through.

• Perfect is the enemy of good. We have no idea how much time the submitter has and how many feedback cycles it will take to get things exactly as we want them.

• Once you've engaged in discussion/review, try to see it through to the end. This is why pull requests aren't just a track with team members rotating through. It's important to have a consistent voice and context retained through the feedback cycles of a PR.

• As long as no regressions are introduced elsewhere, it's not the worst thing in the world if we pull in a feature that is a bit hokey or unfinished. More PRs can be submitted later - contributors deserve a feedback loop, too.

PR Checklist

For each PR, check for the following:

• Does this change enable an Anti-Pattern?

  • If so, let them know in the kindest way possible and suggest an alternative approach.

• Is this a bug fix, or a feature?

  • Apply the appropriate label. This determines whether it will be in the release notes.
  • If it's just a typo fix, don't bother.

• Does the change fit our product vision?

  • This is less obvious and more of a gut feel.
  • Try to understand the root of their need. Sometimes people cut straight to a PR without giving us a chance to understand their use case and come up with the "Concoursey" solution.
    • Sometimes the customer is WRONG. If there's an alternative way to accomplish their root goal, let them know and ask if it makes sense and is sufficient.
  • If you're not sure, ask around and discuss with the team.
  • Try to think of any pitfalls of the new feature.
  • If you're not comfortable with determining this, apply the "needs-validation" label.

• Are there tests for their change?

  • If yes, give them a look, but don't be too picky here; literally everyone writes their tests differently. It's one thing to have a consistent style and it's another thing to force a feedback cycle over it.
    • Do they sufficiently reduce risk? Are there any critical paths without test coverage? Is everything faked out and then never tested at the integration point?
    • If you have genuine concerns, you should absolutely ask about it and/or request changes - test debt is no joke.
  • If there are no tests, have we enabled them to write the tests?
    • If so, ask them to write tests and apply the "needs-tests" label.
    • If not, consider merging anyway and think about how we could improve this. We don't want to force people to come up with their own testing strategy.
  • Are there tests that should be added in a broader context, e.g. Testflight or TopGun?

• Do the tests pass?

  • If not, was it for a good reason?
    • If so, let them know.
    • If not, what can we do to make us trust the PR pipeline more?

• Does the feature work?

  • This can sometimes be tricky to test manually. If you don't feel like you have the time or expertise to determine this completely, that's OK. Ask if they've tried it out themselves.

• How does the change itself look?

  • Again, try not to be too picky. It may not be how you would have written it, but is it really worth the feedback cycle? No code is ever permanent.

  • Watch out for code that could cause the ATC to panic/blow up.

    • Skim for unchecked errs, potential nil panics, or explicit panic() calls.

  • Watch out for mistakes that could lead to a security exploit or leak.

  • Are they logging conventionally?

    • Look for any rogue print statements or log lines from debugging and ask for them to be removed.
    • messages-should-look-like-this, failures should be phrased like failed-to-do-x. Lager messages aren't sentences.
    • No format strings in Lager messages! Use lager.Data instead.
    • Pass around a lager.Logger, don't have a long-lived logger on a struct if it can be avoided.
      • Even better, use lagerctx.

  • Pay special attention to changes that affect people upon upgrading without even opting-in. If something's on by default, or controlled by API/user input (e.g. pipeline config), it could result in widespread breakage or leaks.

• Are there any TODOs? As a rule of thumb, they probably shouldn't be merged in, and definitely shouldn't be merged in without at least discussing it.

Merging

• Go ahead and merge as normal, don't worry about squashing or avoiding a merge commit, but you can if you want.

  • GitHub lets you resolve merge conflicts in the UI, but it can be a bit janky sometimes and makes you resolve it twice (I don't know why).
  • Feel free to merge via the commandline instructions, too. This could also be a good time for "quick fixes" if you noticed small things that would be OK to fix/clean up without asking.

• Are there any docs to be updated or written?

  • If so, you don't have to add them right now, you can just add the "needs-docs" label.

• Are there any BOSH properties or K8s configs that should be added to expose the feature?

  • If so, you don't have to add them right now, you can just add the "needs-exposed" label.

• Are there any issues that the PR "fixed/closed" but didn't actually fix/close?

  • If so, go close them and reference the PR.

• Each pull request should have the appropriate labels before merging. One of (or multiple if necessary) the following labels is required to be attached to each pull request:

  • breaking: this means that the pull request contains a breaking change or, in other words, backwards-incompatible. An example of this would be the new algorithm in v6.0.0, where it introduced a change in behaviour in how versions would be determined for the next build of a job. Normally we try our best to avoid introducing any breaking changes, but if it happens it would mean a major version bump in the concourse version.
  • enhancement: another word for feature, where we are introducing something new that adds on to the existing concourse functionality. An example would be a new fly command.
  • bug: a pull request that fixes an existing bug within a previous released version of concourse would be labelled as bug. An example would be a pull request that fixes a bug in concourse that caused the web nodes to panic.
  • misc: no changes to the existing concourse functionality or behaviour are introduced. For example, a README.md change or refactors that do not change any behaviour.

  These required labels are necessary for the generation of release notes when a new version of Concourse is released. Each pull request will be put into the section associated to its label (eg. a pull request labelled with bug will be put into the Bug section of the release note).

  If you want your pull request description to be placed at or near the top of its section, you can add a priority label to the pull request. You would normally want this if the pull request introduces some behaviour that is of high importance within the release.