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

FIX Don't fail merge-ups if we can't dispatch CI #27

Closed GuySartorelli closed 1 year ago

GuySartorelli commented 1 year ago

Some repositories don't have or need CI, but their merge up jobs are failing because there's no CI workflow. e.g. https://github.com/silverstripe/developer-docs/actions/runs/6158412437/job/16711131792

Continue on error documented here: https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idstepscontinue-on-error

Issue

GuySartorelli commented 1 year ago

The merge ups have already happened by the time it gets to this point. And these steps don't want for CI to run, they only tell CI to start running.

So basically all this change does is say "If you can't start CI on those branches, don't report the merge ups as failing, because the merge up has already happened successfully by the time you get here"

GuySartorelli commented 1 year ago

I am very much against a ci.yml that does nothing - we've already got it in a theme (I think watea?) and that has caused problems where I assumed things were building nicely, but they in fact were not building nicely and the CI being green was meaningless.

That's obviously less of an issue in dev docs, but it still ends up looking like things are being checked when in fact nothing is being checked. It's an anti-pattern I'd rather we stop doing where it is being done rather than start doing in new places.

emteknetnz commented 1 year ago

We've already have an established workflow of "A CI that does nothing" on most of the gha-* repos which using "action-ci" where if there isn't any phpunit then that's OK - https://github.com/silverstripe/gha-action-ci/blob/1/.github/workflows/action-ci.yml#L40

We still need to have CI there because we have an established workflow of doing a patch release immediately after a successful CI run on a module. Do the CI in those instances is a bit of misnomer because it's really just a patch-releaser.

I guess dev-docs is a bit special because it doesn't do patch releases so the "CI that does nothing" doesn't really make sense there. I guess just have a hardcoded string "developer-docs" somewhere and just "continue" instead of looking a ci file in that instance?

GuySartorelli commented 1 year ago

The gha repositories are a neat and tidy bundle that we can say "gha repos behave in this way" - so it's okay for them to be a bit different, provided it's consistent. What's more, that CI isn't strictly "do nothing but pretend you did" - it's "check if there's something to do and do it if there is". which is very different in terms of expectations during peer review.

What sort of issues are you worried about that this change might hide?

I'm not against having a hardcoded "ignore dev docs and userhelp" line in the dispatcher or in here (which were you suggesting? I'd say probably just don't trigger the dispatcher at all fo those two?) but this is also affecting frameworktest... should we be adding CI to that or adding an ignore line for that repo as well?

emteknetnz commented 1 year ago

What sort of issues are you worried about that this change might hide?

Unforseen failures? This PR is saying that failures in the gha-trigger-ci action are OK. That's not a great approach, if there are any failures they should be surfaced.

There's a PR (seemingly in the wrong issue :-) to add CI in frameworktest https://github.com/silverstripe/silverstripe-frameworktest/pull/150, presumbably my brain was solving this same issue for frameworktest at the time

GuySartorelli commented 1 year ago

Discussed offline with Steve - we'll change the exit 1 to exit 0 in gha-trigger-ci instead.