OCA / oca-addons-repo-template

OCA Repository Template
MIT License
61 stars 89 forks source link

Codecov blocking CI jobs #250

Closed sbidoul closed 5 months ago

sbidoul commented 5 months ago

Since yesterday, the codecov action step is blocking all our CI jobs.

This blocks basically everything as there is a timeout of several hours on GitHub actions.

Issue submitted to codecov here: https://github.com/codecov/feedback/issues/354

Things to try

  1. find a way to cancel all queued jobs, so we can try new things without waiting for the job timeouts
  2. try if the latest codecov action (v4) works
    • if not, remove the codecov step from this template...
    • if it works, update this template
rousseldenis commented 5 months ago

I think they removed their v1 actions:

image

IMHO, we can switch all the repos to v3 (v4 requires TOKEN configuration)

sbidoul commented 5 months ago

I'm working on cancelling the jobs right now.

If anyone wants to submit a PR to upgrade to codecov-action v4, under a copier question, that would be helpful.

sbidoul commented 5 months ago

Now cancelling all queued jobs:

import json
import subprocess

def get_repos(org: str):
    res = subprocess.check_output(
        ["gh", "repo", "list", "--limit", "1000", "--json", "name", org]
    )
    for repo in json.loads(res):
        yield f"{org}/{repo['name']}"

def get_queued_jobs(repo: str):
    res = subprocess.check_output(
        [
            "gh",
            "run",
            "--repo",
            repo,
            "list",
            "--status",
            "queued",
            "--json",
            "databaseId",
        ]
    )
    for job in json.loads(res):
        yield job["databaseId"]

for repo in get_repos("OCA"):
    print(repo)
    for job in get_queued_jobs(repo):
        subprocess.run(["gh", "run", "--repo", repo, "cancel", str(job)])
yajo commented 5 months ago

v4 requires TOKEN configuration

Can this be done adding a organization-wide secret? In such case, I guess it's better to update to the newest version, now that we do it.

sbidoul commented 5 months ago

Can this be done adding a organization-wide secret?

Yes absolutely. I have already added an org wide secret named CODECOV_TOKEN.

See example here: https://github.com/OCA/mis-builder/pull/606

This does not block anymore but fails to find the coverage report, though.

rousseldenis commented 5 months ago

If anyone wants to submit a PR to upgrade to codecov-action v4, under a copier question, that would be helpful.

I'm on it

SirAionTech commented 5 months ago

Can this be done adding a organization-wide secret?

Yes absolutely. I have already added an org wide secret named CODECOV_TOKEN.

See example here: OCA/mis-builder#606

This does not block anymore but fails to find the coverage report, though.

Didn't know there was this issue too, so I'll stop bothering in discord (https://discord.com/channels/737652535149592587/761225067798462465) and start here :smile: just picking up from latest Discord messages:

There was no coverage in that repo before codecov's issue either: https://github.com/OCA/mis-builder/actions/runs/8752294448/job/24019628584 In l10n-italy we had a coverage file a few days ago (https://github.com/OCA/l10n-italy/actions/runs/8830773049/job/24244617182), beside uploading issues; so I have re-opened https://github.com/OCA/l10n-italy/pull/4121. Let's see how it goes

..and it went well, see https://github.com/OCA/l10n-italy/actions/runs/8892070599/job/24415363278.

Also note that the token configuration is not apparently needed because https://github.com/OCA/l10n-italy/actions/runs/8892015940/job/24415185936 is successful too, but I suppose explicit is better than implicit :wink:

sbidoul commented 5 months ago

Also note that the token configuration is not apparently needed

It says info - 2024-04-30 09:16:08,581 -- The PR is happening in a forked repo. Using tokenless upload.

So we need to check it works without token on ocabot merge and in main branches too.

SirAionTech commented 5 months ago

Also note that the token configuration is not apparently needed

It says info - 2024-04-30 09:16:08,581 -- The PR is happening in a forked repo. Using tokenless upload.

So we need to check it works without token on ocabot merge and in main branches too.

Right, so we have to use the token, thanks for pointing that out :) Actually it seemed weird that token was not needed, since codecov/action says it is required in https://github.com/codecov/codecov-action/blob/dad251dcaf4fdaa10dfaa1c32aab58f9cb23a448/README.md?plain=1#L39-L46

- uses: codecov/codecov-action@v4
  with:
    fail_ci_if_error: true # optional (default = false)
    files: ./coverage1.xml,./coverage2.xml # optional
    flags: unittests # optional
    name: codecov-umbrella # optional
    token: ${{ secrets.CODECOV_TOKEN }} # required
    verbose: true # optional (default = false)
dreispt commented 5 months ago

So, we have a working PoC, correct? Anyone going for the PR?

SirAionTech commented 5 months ago

So, we have a working PoC, correct? Anyone going for the PR?

Yes, the PR already exists: https://github.com/OCA/oca-addons-repo-template/pull/251.

pedrobaeza commented 5 months ago

PR merged, so now a release of the template should be done and do a massive repo update.

sbidoul commented 5 months ago

I'll do that today.

pedrobaeza commented 5 months ago

@sbidoul I think you should run it automatically also for 15.0, not only 16.0 and 17.0

sbidoul commented 5 months ago

yesyes, and all other branches from 11...

Problem is that with all the repos/branches where pre-commit does not pass, I'm creating PRs for PSC to review, but until that is merged, the broken codecov will remain present and blocking.

I'll look at patching the codecov step only, but that may create copier conflict later.

What a mess...

pedrobaeza commented 5 months ago

OK, I see. I'm starting to fix incorrect branches, as you can see in https://github.com/OCA/account-payment/pull/726. The lack of "line too long" check on 17.0 branches when switching to ruff is doing a lot of harm...

sbidoul commented 5 months ago

The lack of "line too long" check on 17.0 branches when switching to ruff is doing a lot of harm...

Sorry about that. I apparently had forgotten to push that last change back then.

pedrobaeza commented 5 months ago

Well, better later than never. Let's go with it!

HaraldPanten commented 5 months ago

Thanks @sbidoul for your amazing job 💪

pedrobaeza commented 5 months ago

@sbidoul can you please cancel old Codecov jobs that are still blocking actions for the dotfiles PRs?

sbidoul commented 5 months ago

@sbidoul can you please cancel old Codecov jobs that are still blocking actions for the dotfiles PRs?

There are none at the moment.

pedrobaeza commented 5 months ago

OK, then maybe it's just the regular actions corresponding to the pushes. I'm waiting for example this PR to start its actions: https://github.com/OCA/account-invoicing/pull/1718

sbidoul commented 5 months ago

16 and 17 are done. 14 and 15 in progress.

yajo commented 5 months ago

16 and 17 are done

It doesn't look like so. AFAICS the timesheet 16.0 repo still hasn't the update.

I don't mind updating the template myself, but I'll wait for your answer. I just wanted to warn you in case something went under the radar in your process.

Thank you very much @sbidoul. I hope #253 smooths future problems.

rousseldenis commented 5 months ago

It doesn't look like so. AFAICS the timesheet 16.0 repo still hasn't the update.

@yajo Look at : https://github.com/OCA/timesheet/pull/672

Should be treated manually

pedrobaeza commented 5 months ago

I have fixed and merged all the remaining pull request that require manual intervention except the connector ones (as they have serious incompatibilities that require in-depth knowledge to be fixed or bypassed), so this should straighten future contributions on 14.0-170.0 branches.

dreispt commented 5 months ago

Thank you Pedro!

sbidoul commented 5 months ago

In the meantime, codecov has fixed the issue, so I think I'll leave the older branches alone for the moment.

pedrobaeza commented 5 months ago

OK, better to not open the Pandora's box...