Removing the "one commit" check in the CI

Development
#1

Recently a PR was opened to have pulp_ansible disable the “single commit check”. tl;dr I propose we remove that altogether and have Pulp repos use squash at merge time instead.

History

We added the single commit check so that when a change was merged we’d have a single commit if it needed to be backported.

What problems does it create?

  1. It’s a friction for contribution. Folks have to squash their changes locally with every push to make it fully pass the CI. The more friction at contribution time the less contribution happens.

  2. It doesn’t maintain a record of the work on a PR as it really happened.

What alternative can we pursue now that we didn’t have before?

Back then github has a “squash” option, but we had little to no control over what the commit message would read as. This was problematic both for clarity purposes but also for relating issue to Redmine. We don’t use Redmine anymore and github relates issues for us reliably now.

Additionally, github recently added a feature that allows us to control what the content of the commit message is.

Proposal

First let’s have pulp_ansible try this change out. If it goes well, let’s remove the single-commit machinery from the plugin_template entirely and have all repos configured to only use “Allow squash merging”.

#2

I’d love to see this happen. It was practically impossible for me to review the RBAC pr last year (which was a huge, complicated PR that keep evolving) because I couldn’t easily see the changes that had been made since my last review. I think this change would really make it a lot easier to review big changes like that one.

#3

The fact that we cannot assert our test on the resulting commit message on a squash merge is really what drives the single commit check. I’m still not convinced that this will be solved in a bullet proof way. GH rewriting my commits (apart from a simple rebase) is always making me nervous.

Can we maybe assert that the single commit is provided for bugfixes (the only thing we ever backport.) and accept multi commit PR’s for everything else?
If we have good rules, can we come up with a ready-to-ship workflow that checks all those things? While we are on it, can we find a way to remove the [noissue] marker (can we set labels from the workflow to indicate things a maintainer should be very careful with before merging?)?

In pulp cli we have a “ready-to-ship” test that certain phrases like “TODO” are not in the commit message So a dev knows there’s something to come back to.

#4

I think “limited trial and see how it goes” works for me - I see that x9c4 has some thoughts on the issue, we can maybe look at them in light of actual-experience.

#5

I’d like to try something really simple, so I’d like (if we can) to not replace one check with another. In addition to the CI complexity it still requires bugfixes to have the same downsides of a rebase with every push just like existing contributions. This is one of the downsides I’d like to eliminate altogether.

Getting the squashed commit message is key. Of the options Github gives us, I think the cleanest is to use the github PR title and description. At least we know know exactly what we get there, and both the PR opener and project maintainers can edit that.

Can we try this with pulp_ansible? We can operate it for a week or two and worst case we’ll learn and decide we want to reenable the check in CI.

1 Like
#6

That is exactly my concern. It can be edited after the ci turned green on a commit. That is why a commit message is part of the cryptographically secured payload of a commit in the first place.