pubgrub-rs / pubgrub

PubGrub version solving algorithm implemented in Rust
https://pubgrub-rs.github.io/pubgrub/pubgrub/
Mozilla Public License 2.0
337 stars 30 forks source link

Fix CI for conventional commits #139

Closed Eh2406 closed 8 months ago

Eh2406 commented 8 months ago

This project insists on using conventional commits for everything on the dev and release branches. We attempt to check this in CI. The current CI code checks that every commit to a PR or on dev or release follow the style. We also generally use a "squash and merge" merging strategy.

This doesn't work for at least three reasons:

I think this can be fixed by the following:

  1. Turn on merge queues. This feature is intended to allow testing multiple PR's at the same time in the exact state they will be after merging. We don't need the additional parallelism, but we do need CI to run after the commit has been squashed.
  2. Reconfigure CI to run on pushes to PR and being added to merge queue.
  3. Set the conventional commits to only run in a merge queue.

It would be wonderful to have someone test the proposed configuration in some other repo to make sure it works, and then submit a PR to change CI here.

If we don't find a good solution, I would like to remove the conventional commits checking. Force pushing to dev is worse than having to build the first draft of the change log manually.

svanderbleek commented 8 months ago

Hi, is this what you were thinking? Add merge_group to on and have an if condition on the job: https://github.com/mindbikes/pubgrub/blob/release/.github/workflows/ci.yml

svanderbleek commented 8 months ago

Here is the expected skip: https://github.com/mindbikes/pubgrub/actions/runs/6758099716/job/18369373694?pr=3

But here is a merge queue run that also skips: https://github.com/mindbikes/pubgrub/actions/runs/6758112545/job/18369401442

It seems the if statement is not correct.

Eh2406 commented 8 months ago

Thank you for doing the experiment! I'm glad you found out the complications before I merge changes! That is what I had in mind. It's a shame it's not working. A little googling found https://github.com/orgs/community/discussions/51120

aleksator commented 8 months ago

I had a stab for it with merge queues, enabling branch protection rules as recommended in https://stackoverflow.com/questions/76655935/when-does-a-github-workflow-trigger-for-merge-group-and-is-it-restricted-by-bran Basically I reproduced the experiment run by @svanderbleek with those additional settings - same result, didn't work.

Maybe there are some additional things one can do in YAML with checks_requested, mentioned in https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/managing-a-merge-queue

As a workaround, manually squashing commits in the PR can be done before merging. When there is only one commit Github ignores PR title and uses commit message as is.

Another possibility is to look into bors, whether it can do it.

Although at this point I'm not sure if the benefits outweigh those little annoyances. Maybe disabling it is the way to go. Unfortunate.

svanderbleek commented 8 months ago

@Eh2406 I tried something from the support response in that discussion and it worked 👍

PR: https://github.com/mindbikes/pubgrub/actions/runs/6758112545/job/18369401442 Queue: https://github.com/mindbikes/pubgrub/actions/runs/6763774902/job/18381295181

Want me to make a PR?

To enable merge queue you have to go to settings > branches > branch protection rule and make one with Require merge queue checked.

Eh2406 commented 8 months ago

That looks like it worked! Yes, I would love to see a PR!

aleksator commented 8 months ago

@svanderbleek Sure, please make a PR.

Will we also need to enable settings for "Require status checks to pass before merging" with "Commit messages follow project guidelines" under "Status checks that are required"?

aleksator commented 8 months ago

Will we also need to enable settings for "Require status checks to pass before merging" with "Commit messages follow project guidelines" under "Status checks that are required"?

Haven't tested without those settings, but it works as intended with them after the PR. Thanks @svanderbleek.