Flathubbot before merging a build is complete

flathub-infra / flatpak-external-data-checker

A tool for checking if the external data used in Flatpak manifests is still up to date

GNU General Public License v2.0

115 stars 35 forks source link

Flathubbot before merging a build is complete #167

Open Pobega opened 3 years ago

Pobega commented 3 years ago

https://github.com/flathub/com.fightcade.Fightcade/pull/38

It seems that after a build fails the trigger for Flathubbot merging is broken. In my case a build fails because I upstreamed a patch, when I pushed the removal of that patch Flathubbot correctly triggered the next test build but automerged my PR before the build was complete.

I'm unsure if this is intended behavior, if it is feel free to close this.

If this is the wrong repository for Flathubbot issues please point me in the right direction, I couldn't find a repository for Flathubbot specifically.

Here's a screenshot to show the context in case it becomes unclear after the build passes.

gasinvein commented 3 years ago

This surely isn't intended behavior, we explicitly do a check if build was successful before merging. The check is based on GitHub CI status of PR, so this looks like an unfortunate coincidence - probably f-e-d-c found the PR before CI status was updated to "running" after successful sources download phase.

gasinvein commented 3 years ago

Here is the log line:

+ 2021-04-19 12:43:58,760    INFO src.main: PR passed CI and is mergeable, merging https://github.com/flathub/com.fightcade.Fightcade/pull/38

It comes from here: https://github.com/flathub/flatpak-external-data-checker/blob/c5c89339d808c68b4d47088c3998ce1d386b0111/src/main.py#L224-L227

So, to my understanding, the only way for this to happen could've been that the PR CI status was marked as successful at the moment f-e-d-c was checking it.

This is unlikely but serious issue, we should think about preventing it. @barthalion, can you please educate me when and how PR CI status is updated during flatpak build?

Pobega commented 3 years ago

Due to the timing I'm curious if it's related to the download-sources CI passing (since the CI seems to be split into both a download-sources and builds/x86_64)

gasinvein commented 3 years ago

Yes, I believe it is. Looks like there is some time frame between finishing sources download and starting builds, during which CI status is already set to "success" but not yet changed back to "running".

wjt commented 3 years ago

Could the bot explicitly check that there is at least one builds/* jobs and all builds/* jobs have succeeded?

gasinvein commented 3 years ago

Seems a bit hackish but doable. Yet I'd like to avoid flathub-specific code, if possible.

barthalion commented 3 years ago

I guess checking if any job has actually run would be enough here, no need to make it specific to Flathub.

gasinvein commented 3 years ago

Such check will behave same as the current one: sources download job already completed, build jobs not started yet, so there are only completed jobs and the check will pass.

barthalion commented 3 years ago

Ultimately, I'd say that there's nothing wrong about Flathub-specific code given Flathub is primary "customer" of the tool. An alternative would be to require status checks to pass across all repos:

…which I believe would ensure the PR is only mergeable when chosen checks passed.