Sync: Make a branch for merging

[ewels]

As discussed on Slack after the last automated sync, some pipelines seem to have very clean PRs where others have massive ones where the sync tries to delete loads of custom files and all sorts.

Some of this is just due to larger / older pipelines with more variance from the pipeline I think. But there was suspicion on Slack that it could also be pipelines where commit history from dev has in the past been merged into TEMPLATE (x-ref nf-core/eager#615 (comment)). I guess that this is happening when people fix merge conflicts on the automated PR, which will merge dev into TEMPLATE.

So, idea 💡 for making merging easier for people / simpler to keep the TEMPLATE branch clean: How about we push the new commit to TEMPLATE and then make a new branch from TEMPLATE and create the PR to dev from there? Then the merge conflicts can be resolved on github.com without changing anything in TEMPLATE and that branch will stay super clean. This is basically what I do manually (via a fork usually) when I merge sync PRs myself.

[Zethson]

Sounds like a good idea to me.
Maybe the Github API would also allow to prevent merges from any other branch into the TEMPLATE branch (aka branch protection)?

[ewels]

Yes, good idea - we already use branch protection quite a bit, should be easy enough to add to https://nf-co.re/pipeline_health

https://github.com/nf-core/nf-co.re/blob/master/public_html/pipeline_health.php

[ewels]

I was thinking that we need to be a little more clever with how we set this up too, as building on existing open PRs might be difficult. Maybe we take this strategy?

• Close any PRs open from branches called nf-core-template-merge-*
• Maybe: Delete if identical to TEMPLATE but leave if not?
• Add new sync commit to TEMPLATE
• Create a new branch from TEMPLATE called nf-core-template-merge-1.12 - exit if fail / already exists
• Open PR from this new branch to dev

Basically I think that we probably need a new branch for each tools release and can't just update the existing PR as we do currently.

[ewels]

Release of 1.12.1 seems to confirm our suspicion that having clean TEMPLATE branches is the way to go for simple sync PRs - a number of pipelines that manually cleaned / reconstructed their TEMPLATE branch seem to have solved the messy sync problem 👍🏻

So I think this paves the way for this issue 👍🏻

[Zethson]

@ewels don't see it mentioned here (maybe I am blind):
If an already existing sync PR were open (from nf-core-template-merge-..), then this one should be closed in favor of the new one (nf-core-template-merge-.. > former).

[KevinMenden]

I took the liberty to give this a try at #821

I am very much a greenhorn when it comes to using the python github API :) so let me know if there is an obvious better way to do this. So I just made a draft PR to have a start at this.

When testing locally it has the desired behaviour:

• Check any open PRs from nf-core-template-merge-* branches to dev
• Closes them if any are found
• Update and push the TEMPLATE branch
• Checkout new branch from TEMPLATE
• make PR from that merging branch

What bugs me right now is that it fails when the branch already exists - that's something I'll try to catch next.

I removed the updating of existing PRs - in my logic that's not necessary anymore, but correct me if I'm wrong?

[ewels]

Yes we can't update existing PRs if we are using new branches each time. I don't love that auto-closed PRs could have ongoing comments and work etc, but I think that it's something we have to live with. If we can make an automated comment explaining why it is closed and linking to the new PR I think it should be fine.

[ewels]

ps. If the existing branch exists - maybe we should just create a new branch with a -2 suffix etc? As I guess the point is to make new branches for each sync, and this is a non-destructive way to do that.

[KevinMenden]

Okay yes that would be a solution.
We could still close the PR in that case, as we wouldn't delete anything (at least not code), but leave the branch alone.

[KevinMenden]

Alright, so now closed PRs get an update of title and body indicating that they have been closed because they are outdated.

If a branch exists, it gets the suffix -2, if that exists as well, it gets -3 and so on.
I will open the PR for review now, maybe easier to continue discussion there.

[maxulysse]

For the TEMPLATE branch protection, this is what I implemented on Sarek:
I just added the Restrict who can push to matching branches property to @nf-core-bot

[ewels]

Solved in #821