Optimistic Merging Guidelines

[rdebeasi]

Optimistic Merging Guidelines

We've discussed "optimistic merging" for pull requests on this project, but there are some questions on what exactly that term means.

To address that disconnect, I'd like to propose some specific guidelines. If there's consensus on these ideas, I'll write up a document explaining our decision and submit it as a PR.

Background

Optimistic merging was first proposed a few years ago by Pieter Hintjens of the ZeroMQ community in a blog post called Why Optimistic Merging Works Better. That project's Collective Code Construction Contract (C4) is an expanded and formalized version of the same idea.

Here's the workflow as I understand it:

1. To propose a change, log an issue.
2. Seek consensus on the value of that change.
3. Create a pull request to make the change described in the issue.
4. The maintainer checks to see whether the change is a "correct patch".
5. If the PR is a "correct patch", the maintainer merges the PR.

A "correct patch" is one which:

• Solves one "identified and agreed problem"
• Doesn't break APIs (see "Evolution of Public Contracts")
• Compiles cleanly and passes tests
• Clearly explains the problem and the proposed solution

Examples

For some examples of how optimistic merging happens in the real world, see ZeroMQ's closed pull requests.

Contributors make small PRs that each solve one agreed-upon problem. The PRs include detailed commit messages documenting the problem and solution, usually with a reference back to the associated issue.

In exchange, maintainers merge correct patches quickly and give clear, concise feedback on incorrect patches. The patch might be deemed incorrect if tests fail or the PR doesn't follow C4. In that case, contributor either makes the requested changes or closes the PR.

Both contributors and maintainers do some extra work to make this rapid feedback loop happen, and everyone checks their egos at the door. Landing a patch isn't a fight; it's more like a careful dance.

What does this mean for the Library?

There are of course a few differences between ZeroMQ and the Open Practice Library. ZeroMQ is a large, mature piece of software. The Open Practice Library is young (but growing!) project that's a mix of content and code. ZeroMQ has a formal release process; the last stable release was 4 months ago. The Open Practice Library has no formal release process; the site is automatically deployed as soon as a PR is merged. The last release of the Library was - checks watch - 20 minutes ago.

I like the speed with which we're shipping changes and don't want to slow down, but the trade-off is that we have to take some care. If a ZeroMQ maintainer merges a bug, the community has time to fix the bug before release. If we merge a bug, it's immediately shipped to production.

You can't write automated tests for practices, of course. Instead, I propose that we use the Contributing Guide as a sort of "unit test for content." If the "tests" in this guide fail, the PR must be modified so that they pass before it can be merged. If the tests pass, the PR can be merged immediately.

We can (and should!) write tests for our code, but work on our current iteration began last week, and as a result we currently have a test coverage of 0%. Right now, the project is changing so rapidly that we'll probably invalidate tests almost as quickly as we write them. I propose that we use a combination of automated and manual testing for code PRs.

We recently added automated deploy previews as a first step towards a full CI/CD pipeline. As our codebase stabilizes and our test coverage increases, we'll aim to gradually reduce the amount of manual testing to zero.

tl;dr

With the considerations above, I think the Optimistic Merging/C4 guidelines apply pretty nicely to our project. Well-formed patches are accepted quickly. If a patch fails CI or doesn't solve an agreed-upon problem, the maintainer provides kind, constructive, clear feedback, and the community moves forward.

A review is a rapid feedback loop, not meaningless bureaucracy or a rubber stamp. To misquote Mark Zuckerberg, move fast and don't break things.

I'd very much like to hear the community's opinions on this topic. Please feel free to leave comments on this issue.

Thank you for your time, and be excellent to each other.

[springdo]

@rdebeasi - I think this is good stuff! Great stuff in fact and maybe the resolution to this item as it's raised as an issue, we capture the workflow you're suggesting in a the / a development guide. I think we should only do that once some consensus has been reached on this ticket. Perhaps if people thumbs up if they're cool with this going into our dev guide or thumbs down if not?

One thing I would add to the A "correct patch" is one which above is

• Does not break our Code of Conduct / Development guide

I think item 2 Seek consensus on the value of that change is going to be the hardest to achieve but as long as we are open about what we meant by value and who value measured by I think this is going to be a great flow.

Also to help us with Compiles cleanly and passes tests
By way of test automation we can maybe look to using nightwatchjs or some such? there are other free selenium type as a service things that offer free browser testing for Open Source projects so could be a runner when we get to that stage.

For our MD content, there are Markdown linters available and perhaps we should employ some of those for content that is not created through the CMS and see if it provides any value.

[mtakane]

@rdebeasi i like the initial solution and agree on the flow. The small modifications that @springdo added made it better.

I feel we might want to further clarify or change the word value as it is a convoluted term. Might I suggest merit or benefit?

Now it's ensuring we follow it. good intentions will suffer from bad implementation. Let's make good on the intentions of this workflow and path!

[tenfourty]

This is a great discussion. Some quick thoughts from reading your intro @rdebeasi

• In OM and C4 Issues are problems with a template to outline the problem, anyone can log the issue/problem but PRs are not accepted against it until the issue is tagged as an agreed upon problem, this discussion can happen on chat or forums.
• there are similarities between our master based CD process and ZeroMQ, they also believe that master is always shippable so I'm not sure about the comment that there is time to back out or patch over the issue. This is why CI/CD and a strong API and testing are important - and see the first bullet, agreeing that this is a problem that needs solving.
• ZeroMQ was built on C4 from almost the start where there were fewer contribs and Hintjens attributes it's success to this process - fast merges of good enough patches.
• A key idea behind this process is that you accept commits quickly, to encourage greater engagement and turn committers into maintainers as fast as possible to build a bigger community (while regularly pruning dormant maintainers).
• test coverage is not important, Hintjens argues that the only thing that matters is that you don't break contracts (APIs) and this testing should be robust and extensive around those.

[tenfourty]

Personally, I think we should have a standalone guide or put this in the CONTRIBUTING.md file.

Also as the number of practices grows it would be nice to visually see:

• outstanding issues on a practice - allows users to see where the issues are with a particular practice.
• usage of practice and votes for it of some sort to bubble up best ones, this gets rid of or demotes the practices that are rarely used etc.

Final thought - would this work for the CMS workflow that we have in place and not just for code?

[tenfourty]

Further on this, we can probably provide an issue template and a PR template that incorporate these simple rules as well.

[InfoSec812]

One concern I have is the "consensus" requirement. Before we can move forward, I feel like we need a MUCH better definition or refinement around that. Who determines what constitutes consensus? How many people need to agree? Choosing a percentage of contributors becomes problematic as people join and leave the project. Perhaps we consider stating that X number of people need to find value in the change/addition and we consider that sufficient?

[tdbeattie]

Good conversation here.

I also wondered whether one approach can/should apply to both code contributions and content contributions. One thing to consider is the UX and UI designs that are emerging. The early designs include an "Improve this Practice" button at the bottom of each practice page.

My immediate assumption was that this button would take the user into the CMS and allow them to make updates and additions. That would then be submitted, by the bot, as a PR.

However, the approach outlined above suggests it should actually take the user to submit a GitHub issue and they would have to wait for a period of time to get sufficient consensus to go on to adjust the content and submit for review (thus raising a PR). I'm wondering if this is a poorer user experience and will detract from people adding content.

Likewise, the CMS has a "New Practice" button. Once we have the template of a practice configured, my expectation was that the user submits the new practice into review and the content moderator reviews the associated PR (optimistically merging if it conforms to tests we define). But I guess this would bypass the step of gaining consensus via a GitHub issue before the content is submitted?

[rdebeasi]

That's a really interesting trade-off. Being able to immediately submit a PR is definitely a more engaging experience, and I can definitely see how a complicated process would discourage contributors.

On the other hand, imagine the opposite: a new contributor gets excited and submits a new practice without raising an issue first. The new practice is something that the community thinks isn't a good fit for the practice library: maybe it's an article about the finer points of JavaScript, or it's tied to a project management framework like PRINCE2 or ITIL, or it's a duplicate of another practice.

Do you tell the new contributor, "Sorry, this is out of scope" and close the PR? Do you encourage the contributor to rewrite the practice to suit the Library? Do you just merge it, ignore the contributing guidelines, and include content that disappoints or confuses the rest of the community? At that point, none of the options are really great.

Wearing the implementation hat for a moment, it's also worth noting that although the CMS isn't ready for public use yet, GitHub issues probably are. 😄

[tdbeattie]

Good points and examples @rdebeasi and definitely very good food for thought.

One comment on the example about adding practices tied to a PM framework like PRINCE2, I would actually welcome inclusion of these in the Practice Library as long as the content complies with the template in place (e.g. detailing what, why, how and tied to the relevant element of the Mobius Loop or Foundation layer, etc.). I see this Library as a place for many practices and it's up to the user to select (utilising supporting features) the ones he/she wants to use to achieve their outcome. The loop supports journeys on both Agile and Waterfall approaches.

That aside, I'm still on the fence about whether it makes sense to push every idea of an update down a path of defining details about the update, getting sign off / consensus before doing the work or whether there are some situations where going straight to PR is more pragmatic.

I think @InfoSec812 's point about how many people represents consensus is very important. I also think some kind of target around response (e.g. in 24 hours?) is going to be important to keep the community engaged and keep the value stream's length from idea to value as low as possible.

[rdebeasi]

I really like @tdbeattie and @InfoSec812's points about consensus. Maybe we combine them and say something like, "Submit an issue, give the community a day or two to provide feedback, and as long as feedback is mostly positive, move forward. If you don't get any feedback, feel free to move forward."

I agree that for smaller changes - really any content change that's not creating a new practice - going straight to a PR is probably more pragmatic. For example, fixing a broken link or adding a reference probably doesn't require an issue.

[mtakane]

@tdbeattie something moderators of the content will need to be careful of are changes/updates to content that are actually modifying the intent or the purpose of an existing practice too much. For cases like this, or any that would modify the content of a practice more than rudimentary things like links/spelling/grammar/formatting, I would desire an issue be created first for commentary and then a PR created that suits that need and also support the consensus created (or closing of the issue).

This brings an interesting question on the hardship of having multiple contribution workflows. Could we just have a dual path for updating? One that is asked after clicking the "Improve This Practice" like "Are you making minor adjustments?" or "Are you modifying one or more sections of the practice?".

[tdbeattie]

@rdebeasi - you're suggested text of "Submit an issue, give the community a day or two to provide feedback, and as long as feedback is mostly positive, move forward. If you don't get any feedback, feel free to move forward" is great! I really like that.

[tenfourty]

+1 and the process will be in git, so it can always be amended later!

[rdebeasi]

Could we just have a dual path for updating? One that is asked after clicking the "Improve This Practice" like "Are you making minor adjustments?" or "Are you modifying one or more sections of the practice?"

Can we keep it simple for now and add that branching later if we find that we need it?

[rdebeasi]

I've added the process we discussed to our development guide in #332. Before this PR is merged, I have a couple of questions:

• Do you want to follow this process for code changes only, and consider content workflow separately? Or, would you prefer for to follow this process for both content and code?
• Does this text square with what we agreed upon in this issue?

Merging the PR will close this issue. In the spirit of small, incremental changes, I'd propose that we discuss future changes to workflow in a new issue.

Thank you all for your thoughtful discussion on this! 🚀 💻 🍨

[rdebeasi]

Resolved by #332.