Fed engineer PR reviewer checklist

[rmtolmach]

This is a proposed issue template for federal engineers to fill out before becoming PR approvers.

The idea for this checklist came up at a team lead meeting (meeting notes) and then also brought up at the CoP meeting. Definitely open to feedback!

Link to markdown preview 👀

[little-oddball]

I wonder if we shouldn't same something like Approved by OCTO Leadership or something like that. I could see Bill being one of the main approvers but most likely not the only one?

Or, maybe leave as is for now and we can see how it flows and adjust later if we believe it needs it.

[acrollet]

Perhaps the best alternative is Approved by OCTO Engineering CoP Director - perhaps also the Platform Engineering OCTO lead could approve?

[rmtolmach]

✅ changed it to:

• Approved by OCTO Engineering Leadership (comment with name of approver).

[acrollet]

What does this mean in practice?

[rmtolmach]

As it stands, the Platform is responsible for all code that's deployed, meaning that if I approve a PR from a VFS team that leaks PII, for example, I would get called out (or more likely Clint would 🫣). Taking responsibility for your team's code means you take on that liability. This is my understanding. Did I get that right, @little-oddball?

[acrollet]

I understand that this is a bigger discussion than a single PR, but I would love to see more embrace the red mentality and less focus on blame, both for federal engineers and for platform folks! Perhaps something like:

• I understand that I committing to provide leadership for issues that may arise as a result of changes I approve. I will lead blameless postmortems and champion remediations and improvements that are identified.

Side note: is the idea here for federal engineers to provide expedited reviews as requested, or to become the primary/only reviewers for given teams? (The first was my understanding)

[little-oddball]

We can change the term from blame to responsible if you want. Currently in the end, any code that gets into vet-api, vets-website or devops is Platform responsibility which in turn means any issue that happens w/ that code becomes Platform responsibility to address, etc.

The change to teams being more responsible and accountable to their apps is a movement to shift this needle in several ways... one of which having fed engineers aligned w/ VFS team and owning the code for those apps.

[little-oddball]

As far as fed engineers only being involved with expedited reviews as requested or being the primary/only reviewers.

My understanding is the goal is not just expedited PRs but to move away from Platform having to review all PRs in general. Platform is pretty far removed from the ~50 apps teams or so that are committing code regularly so while we can put eyes on them to effectiveness is going to vary greatly due to limited resources we have to support such a scale as well as having the depth of context related to any specific teams items they are working to deliver on.

[rmtolmach]

I like your suggestion. I'll add it!

Side note: is the idea here for federal engineers to provide expedited reviews as requested, or to become the primary/only reviewers for given teams? (The first was my understanding)

The second one. But we still recommend getting another teammate's review, so the fed. eng. isn't the only review.

[rmtolmach]

✅ changed it to:

• I understand that by approving changes, I am responsible for leading efforts to address any issues that may arise. This includes facilitating blameless postmortems and driving the implementation of necessary remediations and improvements.

[acrollet]

Perhaps the best alternative is Approved by OCTO Engineering CoP Director - perhaps also the Platform Engineering OCTO lead could approve?

[rmtolmach]

How are we tracking if someone fills this out? Are we thinking a fed eng. comes through support and is handed this template to fill out? If that's the case, maybe this is fine and a label and assignee aren't needed.

[acrollet]

maybe you could create GitHub teams managed by the CoPs? backend-cop-federal-reviewers and so on

[rmtolmach]

I ended up not changing anything here. We can make improvements if we need to later 🙂

[mdewey]

QUESTION: Is this only for the API or does this reviewer also include the website as well?

[little-oddball]

The intent will be the other repos as well.

[rmtolmach]

@mdewey good question. I tried to make this template as generic as possible so it could be for the frontend and backend, but I don't think we're quite there yet. i.e. there's no code review guide for vets website.