OCA / oca-addons-repo-template

OCA Repository Template
MIT License
61 stars 90 forks source link

[RFC] GitHub actions: make the dev status check optional #83

Closed sbidoul closed 2 years ago

sbidoul commented 2 years ago

If we deploy GitHub actions on 14 or 13, we should make the development status check non blocking (see for instance https://github.com/OCA/l10n-italy/pull/2468).

This can be done with a copier question.

pedrobaeza commented 2 years ago

But in fact, the error is there, and I think we should repair it already in that versions.

yajo commented 2 years ago

If I'm not wrong, un/setting the status check as optional is done through GH interface and not through code, right?

sbidoul commented 2 years ago

If I'm not wrong, un/setting the status check as optional is done through GH interface and not through code, right?

Maybe ? How does one do that ?

joao-p-marques commented 2 years ago

If I'm not wrong, un/setting the status check as optional is done through GH interface and not through code, right?

Maybe ? How does one do that ?

Not sure if this is what @Yajo was referring to, but I think it is this (Branch protection rules in the settings):

image

image

OTOH, if it is on a job level, it would be done through code AFAICS: https://docs.github.com/en/actions/learn-github-actions/workflow-syntax-for-github-actions#jobsjob_idstepscontinue-on-error

yajo commented 2 years ago

BTW I guess there must be some gh api call to do this as well, but I mean that it's no copier task I'm afraid.

OTOH, if it is on a job level, it would be done through code AFAICS: https://docs.github.com/en/actions/learn-github-actions/workflow-syntax-for-github-actions#jobsjob_idstepscontinue-on-error

But if I'm not wrong, that would put the check as ✔️ which would kill its purpose. 🤷🏼‍♂️

joao-p-marques commented 2 years ago

But if I'm not wrong, that would put the check as heavy_check_mark which would kill its purpose. 🤷🏼‍♂️

Well, not exactly. The step would still fail, but the job will pass, which is what we want. What you mention with the required status checks only applies to full jobs, and this is just a step in a job.

yajo commented 2 years ago

Well I mean the CI status icon as it appears in the PR UI. Would it indicate the warn somehow?

joao-p-marques commented 2 years ago

Well I mean the CI status icon as it appears in the PR UI. Would it indicate the warn somehow?

Not AFAICS. But setting the whole job as optional would end up ignoring other important steps too, such as the actual tests: https://github.com/OCA/oca-addons-repo-template/blob/master/src/.github/workflows/%7B%25%20if%20ci%20%3D%3D%20'GitHub'%20%25%7Dtest.yml%7B%25%20endif%20%25%7D.jinja#L85

One option would be to move that status check to its own job, and mark that one as optional...

sbidoul commented 2 years ago

One option would be to move that status check to its own job, and mark that one as optional...

Yes, we could try that. It's relatively lightweight as it does not need the database.

pedrobaeza commented 2 years ago

I insist: that check is legit and must be complied. Why fade it?

sbidoul commented 2 years ago

Because I'm afraid too many branches will become red if we deploy this on 13/14, and the community will struggle... but we can try.

pedrobaeza commented 2 years ago

I don't think too many will become, as this has stayed on most of the modules with the default, so better to fix those that aren't complying if manually they have been changed.

yajo commented 2 years ago

One way to address this also is to think: how was this working in travis?

If we try to make the travis-to-ghactions transition as "equal" as possible, then no much friction should be introduced. If we're using the opportunity to introduce new checks, then it's when problems can happen. But that can be done later once the 1st transition is finished.

Both approaches have pros and cons, just a suggestion :)

BT-rmartin commented 2 years ago

I would also vote for doing the check on its own job and marking it as optional. Otherwise for old modules, it requires the action on several modules involving their authors/maintainers, as one module could become Stable but the dependencies stay as beta, and right now the check is blocking, when it's something not such critical imho. Example: https://github.com/OCA/delivery-carrier/runs/6116809311?check_suite_focus=true

pedrobaeza commented 2 years ago

The problem is not that one, as the PR is resolving it. It shouldn't be ignored, as the main goal of development_status will be obfuscated.

sbidoul commented 2 years ago

I also think we must make the transition to GitHub actions as friction-less as possible.

So I suggest having a copier question to enable/disable the license and dev status check (so PSCs can choose), and make it mandatory for v15+.

sbidoul commented 2 years ago

This was implemented in https://github.com/OCA/oca-addons-repo-template/pull/144