Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New pattern suggestion: Managing capacity for reviewing contributions #692

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

tsadler1988
Copy link
Contributor

No description provided.

@spier spier added 1-initial Donuts, Early pattern ideas, ... (Please see our contribution handbook for details) 📖 Type - Content Work Working on contents is the main focus of this issue / PR labels Jun 12, 2024
Copy link
Member

@spier spier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one @tsadler1988!

I have seen something similar at work as well, at least in the sense that the act of "reviewing contributions" is being tracked as "real work".

I might have some more practical questions about how you do these things in practice but already wanted to provide a quick review to get the process started.

patterns/1-initial/capacity-for-contributions.md Outdated Show resolved Hide resolved
patterns/1-initial/capacity-for-contributions.md Outdated Show resolved Hide resolved
patterns/1-initial/capacity-for-contributions.md Outdated Show resolved Hide resolved
patterns/1-initial/capacity-for-contributions.md Outdated Show resolved Hide resolved
patterns/1-initial/capacity-for-contributions.md Outdated Show resolved Hide resolved
patterns/1-initial/capacity-for-contributions.md Outdated Show resolved Hide resolved
* Missing communication between contributors and maintainers on expectations/lead time for contributions to be reviewed/released
* Tension in prioritizing InnerSource contributions against other work

## Solutions

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be a good idea to recommend that the contributing team informs the receiving team as early as possible when planning large contributions. One form to operationalize this is through Draft PRs, which allows early feedback, specially if the PR text contain a summary of the planned changes.

Using stacked PRs and slicing the reviews into smaller parts could also help.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I have seen many host teams do is to ask for an issue before creating a PR.

Or in other words, starting the conversation about the why and what (in an issue) before sending the how (in a PR).

We could add a note about this here, if it fits.

And I am even starting to think that this point could work as a new tiny pattern as well a la "Remember to talk to each other before sending code" :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dellagustin-sap with "stacked PRs" you mean breaking up a larger contribution into multiple PRs that should be reviewed in a specific sequence? i.e. first PR1, then PR2, then PR3, ...?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spier , I'm not sure that there is a well understood definition for the term "stacked PRs", but what I meant was something like:

  1. Create a DRAFT PR based on a feature branch explaining the change that is going to be sent
  2. Instead of committing your changes in one go to the new feature branch, send small incremental PRs to the new feature branch, as soon as you have new code that can be reviewed, this way the team can review small pieces until the feature is ready to be merged as a whole

This only works of course if the change can be split into small increments. That's not always the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be a good idea to recommend that the contributing team informs the receiving team as early as possible when planning large contributions.

Yes I will add this. There are a few methods I think worth mentioning: drafts PRs (per @dellagustin-sap), issues (per @spier), RFCs, or even an informal conversation e.g. in Slack

small incremental PRs

Definitely agree this is preferable but goes a little beyond the scope of this pattern. Small increments is IMO best practice for Agile, InnerSource, CD... Perhaps I'll mention this in 'forces' - something like 'contributors already strive to make small PRs, but find instances where larger PRs are unavoidable.'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing changes @tsadler1988!

@dellagustin-sap as you started this particular thread, would you mind giving this a review? And while you are at it, possibly even leaving an official review on the PR, to confirm that I am not the only one approving it.

Cheers :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC we even mention the "please talk to the receiving team before submitting a PR" part in the learning path :)

It's one of the things that I keep telling people even in open source: Don't send a PR as your first line of communication. Nothing is more frustrating than going though learning the project, making the modification work and then being told that the receiving team does not accept it - no matter how good the reason may be.

Copy link
Member

@spier spier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tsadler1988 thank you for sharing this pattern with us.

What I appreciate most about it is the focus on a specific, seemingly minor topic that can significantly impact how "InnerSource collaboration" is perceived within the organization.

In the worst-case scenario, someone might complain, "Oh, I hate these InnerSource contributions; they always disrupt my team's delivery predictability." While I don't agree with that view, if this pattern can mitigate that risk by treating "review time" as actual work, then I'm all for it!

One thing I'm curious about:
Does your team allocate any slack time when planning sprints? That is, do you leave time available for "unplannable work" that inevitably arises? I usually try to keep 10-30% slack time for teams, allowing them to react more flexibly to what happens during the sprint. It might seem like a lot, but it has worked very well for me to keep teams happy and productive.

Overall, I'd be happy to merge the pattern as it is or with minor changes. Once merged as an initial pattern, we can share it more easily with others to gather further "known instances."

Later, we can add visuals and other enhancements to give this pattern more recognition and prominence once it's published in the online book 🥳

Thanks again Tom!

tsadler1988 and others added 2 commits June 24, 2024 17:06
* Maintaining team is given time to review larger contributions in team capacity planning
* Reviewing contributions is prioritized against other work (e.g. in sprint planning)
* Maintainers communicate their capacity for reviewing contributions, the priority of contributions, and an estimate of when a contribution will be reviewed/released
* Maintaining team has a service level objective (SLO) (e.g. 2 working days) for contributions receiving initial feedback
Copy link

@michael-basil michael-basil Jun 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love the SLO because it creates productive back pressure, transparency and utility for the development team, product owner, scrum master and project management personnel.

It also layers into moving in alignment with DevOps / SRE / Observability approach.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on the SLOs - not only does that create back pressure - it also helps communicate to contributors what expectations can be wrt. to response times. Especially inside of companies those can vary greatly between teams.

Btw. what I found helpful for host teams is to track "time to initial response" - https://github.com/github/issue-metrics (I found that metric way more helpful than something like time to close - time to first response is something you can easily optimise. Time to close not so much because minimising that tends to have nasty side effects.)

@michael-basil
Copy link

michael-basil commented Jun 24, 2024

@tsadler1988 - I reviewed this with my colleague Bill Westfall (who I introduced you to in Slack).

I left a couple of comments, but overall I love the pattern. Great to see this drafted cleanly.

Kudos.
🌿


## Resulting Context

Maintaining team understands the overhead of reviewing large contributions and is given capacity to do so. Project manager and product managers are better able to plan, estimate, and track other work in the team by accounting for the time taken to review InnerSource contributions. Contributors understand when their contribution will be reviewed and released, and how long before the maintainers will provide initial feedback.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree that product and project managers are the best people to filter this before a large PR hits the team unaware. Would be curious to chat and understand the intake process better and what part of that may/may not be working right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'm making an assumption that there's a foundation of contributions being valued, and even if they are it could be difficult to prioritise them against other work. Some teams may go to the extreme of having a policy of 'merge every contribution' whereas others will be pickier.

It might also be good to specifically call out that smaller PRs are still dealt with ad-hoc - IMO we wouldn't want smaller PRs being blocked by bureaucracy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That matches my observations as well: Small changes tend to be smooth. It is larger changes, either as one PR or multiple ones that are related that cause tension.

@tsadler1988
Copy link
Contributor Author

tsadler1988 commented Jun 24, 2024

Does your team allocate any slack time when planning sprints?

Good question @spier. When we did Kanban we would just prioritise contributions as they came in. I'm less directly involved with the dev teams these days, but we've recently moved to Sprints so I will ask around.

@spier
Copy link
Member

spier commented Jun 24, 2024

Looks like this might be ready to be merged rather soonish. Awesome!

@tsadler1988
Copy link
Contributor Author

Does your team allocate any slack time when planning sprints?

Good question @spier. When we did Kanban we would just prioritise contributions as they came in. I'm less directly involved with the dev teams these days, but we've recently moved to Sprints so I will ask around.

Looks like we continue to be flexible like we were in Kanban, rather than building in slack. 'Asteroids' as we call them (live issues, InnerSource contributions etc.) might be prioritised during the Sprint, bumping other items originally included in the Sprint. So the Sprint planning is more of a 'this is what we hope to deliver in this Sprint assuming no asteroids'. Perhaps this is a luxury of building software internally - this approach might not fly when building for a customer!

@michael-basil
Copy link

This is always a tension that develops with teams that are successful to get customers.

I have some approaches I use with Agile to start out strict Scrum as a training mechanism at formation and or during stretches when the team is really squishy.

Happy to unpack a conversation around this sometime in a call.

Can be brought into Dojo Circle on Friday at 13:00 UTC see #dojo-circle

Not every conversation can be maximized asynchronously!

@MaineC
Copy link
Member

MaineC commented Jun 28, 2024

Nice one @tsadler1988!

I have seen something similar at work as well, at least in the sense that the act of "reviewing contributions" is being tracked as "real work".

I fully agree. I would like to add that this is not only true for InnerSource but also something that I have seen in open source:

I have seen, often single vendor, open source projects that were trying to increase committer diversity to go beyond the founding company. For that to happen, there needs to be time to review PRs that are not coming from your own company. Often though that clashes with how developers are incentivised: Reviews may not be something that counts for performance reviews. Reviews of PRs that do not pay into the current tech strategy may not be something that counts. If there is time pressure often those things win that provide a short term benefit for one's own business goals wins.

Making mentoring time transparent and something that is explicitly tracked can help with raising awareness for this work. It can help teams understand that this is what helps engineers gain impact beyond their own team and grow as a result. Tracking the number of very active reviewers with metrics can also help raise questions and increase that understanding.

@MaineC
Copy link
Member

MaineC commented Jun 28, 2024

I really like having that here as a pattern - it's something that I have been explaining to teams within InnerSource but also in open source over and over again. I've forwarded the draft pattern directly to our InnerSource teams as it does a really good job at providing a well phrased, easy to understand description.


## Story

The BBC's connected TV application are built by a number of teams, each with different areas of responsibility. They work on each other's areas of the codebase via InnerSource on a regular basis.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A minor comment. Typically in patterns, the story is generalized to not mention a specific company or institution (it's a pattern, so it ideally should be described in ways that can be applied to other companies/users). So, maybe:
An application developed by a company for use by users outside the company is built by a number of teams, each with different areas of responsibility.

The specific information becomes apparent in the known instances reference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1-initial Donuts, Early pattern ideas, ... (Please see our contribution handbook for details) 📖 Type - Content Work Working on contents is the main focus of this issue / PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants