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

Make erroring wave builds fail CI #7370

Closed
wants to merge 1 commit into from
Closed

Conversation

edmundmiller
Copy link
Contributor

So we can know when the arm builds are failing. Reporting hook to come later.

@edmundmiller edmundmiller requested a review from a team as a code owner January 26, 2025 23:36
@edmundmiller edmundmiller self-assigned this Jan 26, 2025
@@ -57,7 +57,7 @@ jobs:
timeout-minutes: 60
strategy:
fail-fast: false
max-parallel: 4
max-parallel: 2
Copy link
Contributor

Choose a reason for hiding this comment

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

why not 4?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why set a limit at all? Idk, I think it was to not soak up a ton of runners.

@ewels
Copy link
Member

ewels commented Jan 27, 2025

Looks like this logic is the wrong way around, meant that only PRs coming from branches on the nf-core repo were being run:

if: github.event.pull_request.head.repo.full_name == github.repository

@edmundmiller I'm not sure that we want to fail builds yet - are we confident enough that it won't block new modules?

@ewels ewels changed the title Allow wave builds to fail Make erroring wave builds fail CI Jan 27, 2025
@edmundmiller
Copy link
Contributor Author

Looks like this logic is the wrong way around, meant that only PRs coming from branches on the nf-core repo were being run:

No that's the right way around because it fails if you try to make a PR from anywhere but the nf-core modules repo. So it should skip.

Ideally, the bumps will come only from the renovate commits. Users will still have to make the initial containers.

@edmundmiller I'm not sure that we want to fail builds yet - are we confident enough that it won't block new modules?

@ewels This PR was because you asked for it 🤣 I think it's fine; it's not required, so it won't block the PR.

@mashehu
Copy link
Contributor

mashehu commented Jan 27, 2025

People get confused about failing tests, even if the tests are not required. Better to skip it/succeed with warning.

@edmundmiller
Copy link
Contributor Author

People get confused about failing tests, even if the tests are not required. Better to skip it/succeed with warning.

Sounds good! Closing this then, and we'll link to it if anyone asks and can revisit it later.

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

Successfully merging this pull request may close these issues.

4 participants