FIX Don't fail merge-ups if we can't dispatch CI #27
Conversation
|
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" |
There was a problem hiding this comment.
Sorry, I've flip-flopped on my earlier recommendation here
I realise now that I don't like this approach because it could be masking any legit failures. I thought about maybe doing something equivalent and change exit 1 to exit 0 here https://github.com/silverstripe/gha-trigger-ci/blob/1/action.yml#L68 though again, that may be masking a missing ci.yml when the module should be one.
I think we're better off just adding a ci.yml like file to dev-docs so that it's consistent with all of the other modules.
Since this is annoying in the context of doing a release, just do manual merge-ups on dev-docs for the time being and we can revisit this later
|
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. |
|
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? |
|
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? |
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 silverstripe/silverstripe-frameworktest#150, presumbably my brain was solving this same issue for frameworktest at the time |
|
Discussed offline with Steve - we'll change the |
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