Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

report build failures as failures #675

Closed
wants to merge 1 commit into from

Conversation

adamcstephens
Copy link

@adamcstephens adamcstephens commented Feb 1, 2024

It is far too easy to miss clear failures when we report them as neutral. Not only does this not register them in GitHub visually as a failure, but once all checks have completed successfully GitHub collapses the checks UI and shows the PR as fully successfully checked.

We're already merging PRs that are failing checks, it's just not as visible. This change will at least provide visibility into those failing checks, to allow us to make better informed decisions when proceeding to merge anyway.

Note: I have little knowledge of what I'm doing, but this seems to be the correct place to change this.

@alyssais
Copy link
Member

alyssais commented Feb 1, 2024

I don't think timed out should be a failure. That's something that happens extremely often (basically any PR targeting staging will time out on Darwin), and getting people used to merging despite a reported failure will cause alarm fatigue and make it more likely that real failures are ignored.

Copy link
Member

@alyssais alyssais left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we distinguish failures of the leaf OfBorg was trying to build from failures of dependencies? Another thing that comes up a lot especially on staging is failures of dependencies due to flaky tests or whatever, and those shouldn't block a PR being merged, so they shouldn't show up as failures.

@adamcstephens
Copy link
Author

adamcstephens commented Feb 1, 2024

Can we distinguish failures of the leaf OfBorg was trying to build from failures of dependencies? Another thing that comes up a lot especially on staging is failures of dependencies due to flaky tests or whatever, and those shouldn't block a PR being merged, so they shouldn't show up as failures.

I can't answer the first question as I don't know enough about the entire system. I don't think PRs are blocked regardless on failing ofborg checks.

@alyssais
Copy link
Member

alyssais commented Feb 1, 2024

I don't think PRs are blocked regardless on failing ofborg checks.

Not on a technical level, but people get very nervous about ignoring a failure when merging, and in practice will very rarely do it without being very sure it's okay. That's a useful property that we should preserve, and if we lose it by accident, we won't get a second chance.

@hraban
Copy link
Member

hraban commented Feb 1, 2024

@adamcstephens
Copy link
Author

Happy to reopen at any time.

@hraban
Copy link
Member

hraban commented Apr 26, 2024

Any reason you closed it, @adamcstephens ? I thought this was a worthwile idea and I hope someone will pick it up someday!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants