Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
6 tasks done
emteknetnz opened this issue Jul 12, 2023 · 9 comments
Closed
6 tasks done

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

emteknetnz opened this issue Jul 12, 2023 · 9 comments
Assignees

Comments

@emteknetnz
Copy link
Member

emteknetnz commented Jul 12, 2023

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

  • We've previously confirmed that the new gha-merge-up action is working correctly on silverstripe/framework
  • Every supported non-recipe has a .github/workflows/merge-up.yml file added to the default branch
  • Every silverstripe/gha-* module has the workflow added too
  • The workflow cron is restricted to the appropriate account i.e. silverstripe, symbiote, etc, so that forks do not run the workflow
  • Recipes do NOT have this workflow added. This includes silverstripe/installer, silverstripe-recipe-core and silverstripe/recipe-kitchen-sink, etc.
  • Merges are run with a 3 day offset from the CI build

Note

PRs

@emteknetnz emteknetnz changed the title gha-merge-up - rollout to all supported modules gha-merge-up - Rollout to all supported modules Jul 12, 2023
@GuySartorelli GuySartorelli transferred this issue from silverstripe/.github Jul 20, 2023
@emteknetnz emteknetnz self-assigned this Aug 8, 2023
@emteknetnz emteknetnz removed their assignment Aug 9, 2023
@GuySartorelli
Copy link
Member

@emteknetnz Please also add this to frameworktest

@emteknetnz
Copy link
Member Author

emteknetnz commented Aug 11, 2023

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
Copy link
Member

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 GuySartorelli self-assigned this Aug 11, 2023
@GuySartorelli
Copy link
Member

@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
Copy link
Member Author

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
Copy link
Member

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

@emteknetnz
Copy link
Member Author

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

@emteknetnz
Copy link
Member Author

@GuySartorelli Have raised a new PR to module-standardiser re logic for reciple-plugin silverstripe/module-standardiser#5

@GuySartorelli
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants