Defining (default) merge policies

[niccokunzmann]

Motivation

This team came together through NLNet based on their own projects. The projects all have different rules of how to merge Pull Requests. This topic is here to provide a guide on how to merge and start us off into clarity.
I create this topic because I asked you @abe-101 @SashankBhamidi @stevepiercy @tobixen to collaborate ❤️

Observations

What I observe:

• on existing projects of collaboration, we know how to merge
  • icalendar
    • lead by Nicco, docs delegated to Steve, 1+ reviews required, tests running
    • after time of inactivity (7+days) as a collaborator, you can merge you PR without review, to maintain progress
  • E.g. caldav & Open Web Calendar - the maintainer merges PRs
• on PyCal, the PRs are still open and it is unclear to me as when to merge them
  

Current Practices

So, what I think, we agree on in most cases, for a merge:

• tests&CI need to run
• people who are responsible for the repository merge the PRs
• we ask people who are knowledgeable for a review to improve the quality

Proposal

We all love tech. I distinct between GitHub rules and mutual understanding. Mutual understanding creates convergence toward agreement while allowing for trial&error with respect to different points of view.

Based on Current Practices, I propose the following process:

1. Each project here gets a CONTRIBUTING.md This states at least:

• Who are the maintainers
• Who can merge and when

2. If this does not exist, we assume that

• the creator of the project
  • is the maintainer of the project
  • merges PRs
  • is responsible to request reviews

Way forward

(1) With the PyCal organisation, we stepped up to build a common project (starting with pycal.org) with common investment, not only individual projects. This requires another level of clarity.

(2) I request your input on this to refine the process.

(3) I want clarity in action for my projects with your understanding and also for pycal.org.

[abe-101]

I'm in favor of a rule requiring at least one review before merging.

[tobixen]

Reviews are always nice, but it does take time and effort. An anecdote from my previous workplace ... in June 2004 we were four-five persons working on a software project which was not set into production yet. Development speed was fast and furious, and if we found some silly bug we would of course just fix it. Five years on, and the development team was 50 persons (I was not among them, but I had access to the code base), and the development was really grinding very slowly. One day I found a really trivial bug, could be solved with a minor fix at one line. I could of course not just fix it and commit, it had to go through change management, including explaining this rather silly little technical detail and how I planned to fix it to a non-technical manager .... sorry, I digress again. :-)

Generally, I think some exceptions may apply:

• Immature projects that haven't passed the 1.0 milestone
• Urgent bugfixes or security fixes (there have been incidents where a critical security hotfix has opened up worse security exploits - reviews are nice, but should not delay the merge)
• Minor fixes, tests added, documentation, etc.

There is also a security dimension. Consider if a password is leaked, or someone does a $5 wrench attack - then it would be nice to know that all changesets have been reviewed. As a security-policy this would probably only be useful if GitHub can enforce such a rule strongly, without admins being able to circumvent it.

[stevepiercy]

For reference, here's what we have for icalendar https://icalendar.readthedocs.io/en/stable/contribute/maintenance.html. The content up until New releases is relevant to this discussion.

There's also some more relevant content starting with https://icalendar.readthedocs.io/en/stable/contribute/index.html#pull-request-requirements.

Although that content is relevant, we don't explicitly state in the documentation when it's OK to merge PRs for icalendar.

---

I'd revise the proposal as follows to keep things distinct.

1. Specify contributing guidelines.
2. Specify the roles of maintainers, collaborators, and code owners, as necessary, and who fulfills each of those roles. By default, the maintainer on PyPI is the maintainer and code owner.
3. Explicitly state who is responsible to review a pull request, preferably through CODEOWNERS.
4. Explicitly state when a pull request may be merged and by whom. These requirements may include:
   • All tests pass.
   • Documentation builds successfully and passes checks. Any changes to documentation should render correctly in the pull request preview on Read the Docs.
   • A change log entry is included, except for trivial matters, such as typographical errors and updating links. Preferably, this would be enforced through CI.
   • All conversations should be addressed and resolved.
   • At least one approval from a maintainer or code owner.
   • Maintainers may override any or all of the requirements, but should add a comment explaining their reason for doing so.
5. Managing stale pull requests. If an open pull request has had no activity for at least seven days, either through a comment or commit by the contributor, then a maintainer or code owner should ask the contributor whether they need help to complete the work. After another seven days, the maintainer or code owner may resolve the open pull request using their best judgment.

[SashankBhamidi]

I like how icalendar is structured under Steve’s approach, and I’d prefer we align our branch protection rules across projects in a similar way, with Steve taking the lead on defining and setting them up.

For the pycal.org site specifically, I’d suggest requiring two reviewers instead of one: Steve can handle typography and content quality, while I or others can focus on design review.

[SashankBhamidi]

I will get to reviewing the open PRs in the website repo soon, I didn't get enough time to do it today morning.

[SashankBhamidi]

And I want everyone to watch https://github.com/pycalendar/pycal.org/ for updates to the repo for Issues and PRs please

[SashankBhamidi]

[tobixen]

I have a contributors guide at https://github.com/python-caldav/caldav/blob/master/CONTRIBUTING.md for the CalDAV library. It seems to be missing on most of "my" other projects.

Some specifics of the caldav library ... and on my personal approach to things:

• The CalDAV libary is very much dependent on integration tests to pass, including integration tests towards cloud providers rate-limiting their API. Since there are a lot of integration tests and a lot of cleanup/setup code it may take several days to do full integration test. Due to this, the releases are infrequent, and it makes sense to have a final round of QA-work just before release, including asking AI for code review comments.
• Whenever there is work that doesn't need the approval of others and doesn't need to go through pipelines/workflows, for me it feels silly to create a pull-request or merge-request. However, I think the general rule should be that all changes on mature projects should go through pull-requests or merge-requests.
• I somehow grew allergic to "Conventional Commits" when we hired a young colleague and he prepended all his puppet commit messages with "chore: ". However, for most python projects it does make some sense - so I've (just recently!) embraced this convention. I'm also trying to use "imperative mood" on the commit headers, as it usually causes those lines to become shorter.
• I don't want to make a "personal fork" of my "own" projects in GitHub, when doing development I think it makes most sense to work on feature-branches directly on the project itself (as long as one has permissions for that).
• Regarding "mature projects" above - according to the SemVer, software <1.0 should not be considered to be stable - personally I think it makes sense to optimize for rapid development for projects that haven't passed the 1.0-milestone - so for those projects I'm likely to ignore or break rules, at least as long as I'm working alone on some project.
• I'm in favor of one logical and independent changeset per commit in the main branch - and I don't like merge commits - hence I always use the "rebase" or "squash"-button when merging PRs/MRs. (If someone can tell me how to use git efficiently on a main branch with merged branches containing lots of micro-commits, I may reconsider this stance, but I think it's "best current practice" to squash feature branches and rebase before pushing to the main branch.
• There are not so many contributors to the CalDAV library - except for stray pull requests from arbitrary users (most of them contributing once only), I'm pretty much alone on the project. I'm not expecting anyone else to have time to do code reviews on it, hence I'm merging my own work as well as work from contributors whenever I feel it's ready. Of course it would be nice if someone has time to look through things. More maintainers and more reviewers would be fine. The original owner of the CalDAV project is still alive and it's possible to reach out to him if my ship should sink in the Black Sea.
• Similarly, I don't have a lot of time to review pull requests in other projects, and I absolutely don't want to be a blocker - if you're missing a code review from me and find it important, then please ping me. I would like to review and have a say on "my" projects, but if I'm AFK or very busy for days, I'd probably prefer that the pull requests are approved without me blocking it.
• I usually try to respond quickly to issues and pull requests.
• Personally I'm sometimes of the opinion that rules are made to be broken. For trivial things like a one-byte typo fix in the documentation, write "thanks!" and merge quickly, without any peer review.
• Personally I believe strict issue templates and lots of rules for contributors to read through may cause significant friction, so I do try to avoid that - but I respect that opinions may differ at this, and I'm not very much against introducing organization standards on my repos.
• After I started using Claude Code, setting up CI-scaffolding and such has become much more easy for me. I do have some personal standards for my projects now, including pre-commit setup stopping me from committing and pushing things too fast - see https://gist.github.com/tobixen/e4df792142a6b0ca33295d8d3e040473

[niccokunzmann]

Thanks for all the responses!

For me, how to proceed:

1. I created a PR for sphinx-icalendar to add the contribution guidelines, mostly copying from steve: add contribution guide sphinx-icalendar#5 you can leave comments if that is interesting to you.
2. I will create a CONTRIBUTING.md file for pycal.org in the same way and that can be commented on.
3. Learning from both, I might modify (1) and then create a policy for my other projects.

[SashankBhamidi]

• I will create a CONTRIBUTING.md file for pycal.org in the same way and that can be commented on.

I think you should also create CODEOWNERS file for automatic review requests.

Else it would be easy to miss PRs