search

silverstripe / gha-merge-up

GitHub Action to merge-up supported branches in a repository
BSD 3-Clause "New" or "Revised" License
0 stars 2 forks source link

gha-merge-up - Rollout to all supported modules #4

Closed emteknetnz closed 1 year ago

emteknetnz commented 1 year ago

Follow up to https://github.com/silverstripeltd/product-issues/issues/753

Currently only silverstripe/framework has a .github/workflows/merge-up.yml file which uses the new gha-merge-up action

This should now be on all supported modules, though not recipes

ACs

Note

PRs

GuySartorelli commented 1 year ago

@emteknetnz Please also add this to frameworktest

emteknetnz commented 1 year ago

We don't actually have proper branches on frameworktest so it's kind of not applicable there e.g. there's no 1.0 branch, there's also no 1.0.0 tag.

I'm not even sure why we're doing releases for it on the CMS 4 line tbh, it's a purely dev module

Merge-ups as is won't work on frameworktest due to the use of a pre stable 0.4 branch, gha-merge-ups assumes that minors branches are whole numbers so it won't be able to merge-up from 0.4 to 1

I'm not proposing make changes to frameworktest to make it so that gha-merge-ups works, given it's just a semi obscure dev only module I think we just don't bother with auto-merge ups for frameworktest

GuySartorelli commented 1 year ago

Any new behat test coverage for CMS 4 which requires changes to frameworktest will require adding those changes to 0.4, which will then need to be merged up. We might (and did this week) need to do this for testing bug fixes.

given it's just a semi obscure dev only module I think we just don't bother with auto-merge ups for frameworktest

This does mean we'll have to remember to merge things up manually - in the best case things will break and it'll be slightly annoying figuring out why but we'll manually merge up. In the worst case, we forget to merge something up and it results in a test passing which should be failing. The latter case is my real concern but if you're confident that's not going to happen then we can leave it for now.

I'm not even sure why we're doing releases for it on the CMS 4 line tbh, it's a purely dev module

It needs tags because there are tags - if it had no tags at all, we wouldn't need any tags because 0.4.x-dev would be used - but since it has tags, the stable tags are installed, so new changes need to be tagged. But that's irrelevant to merge-ups anyway.

GuySartorelli commented 1 year ago

@emteknetnz There's some weird update to an unrelated cron on many if not all of these PRs - please address that before I continue with the review.

Also if any of the PRs include other changes unrelated to merge-ups specifically, please add comments to those PRs explaining what is being done and why, so I don't have to ask.

emteknetnz commented 1 year ago

Core thing here is that the PR creation here was done using module-stanardiser, rather than the usual process of manually creating PRs

Also if any of the PRs include other changes unrelated to merge-ups specifically, please add comments to those PRs explaining what is being done and why, so I don't have to ask.

I'm not going to comment on individual PRs. In all cases changes unrelated to merge-ups are because there's a script in module-standariser that does that e.g. change "SilverStripe" to "Silverstripe" in README.md

There's some weird update to an unrelated cron on many if not all of these PRs - please address that before I continue with the review.

That was a result of using adding this to module-standardiser so that the crons would have a "predictably random" start date. keepalive.yml only need to run even 59 days or less so that crons on actions don't stop if the repo had no activity, so it having a different start date makes no functional difference.

GuySartorelli commented 1 year ago

Changes to module-standardiser merged. Reassigning to @emteknetnz to re-run it since it will affect the existing PRs.

emteknetnz commented 1 year ago

Have re-run module-standardiser and updated all the crons because of the new randomisation logic

emteknetnz commented 1 year ago

@GuySartorelli Have raised a new PR to module-standardiser re logic for reciple-plugin https://github.com/silverstripe/module-standardiser/pull/5

GuySartorelli commented 1 year ago

All PRs merged. It won't work for modules with weird branching strategies but that's because the branching strategies are borked, not because the workflow is borked.