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

Fix the condition to run the nox-cross-arch-all job #265

Merged
merged 3 commits into from
May 6, 2024

Conversation

llucax
Copy link
Contributor

@llucax llucax commented May 3, 2024

The current condition makes the job skip unless there is a failure, which means we also skip on success, which is not right, as we want this job to succeed if the matrix job succeeds.

To fix this, we only skip if the nox-cross-arch matrix job was skipped and we run otherwise, failing the job if the matrix job was not successful. We also use the job name explicitly, as we only have one dependency and it is easier to reason about a string than about arrays.

For consistency reasons, and to avoid future surprises, we also make the nox-all job run on failures of its child jobs, even when it is not necessary for now as we are not skipping jobs in the nox matrix job.

@github-actions github-actions bot added the part:ci Affects the GitHub workflow and other parts for running CI label May 3, 2024
@llucax llucax force-pushed the fix-cross-arch branch 11 times, most recently from 9f4f9c8 to 91132ff Compare May 3, 2024 12:56
@github-actions github-actions bot added the part:template Affects the cookiecutter template files label May 3, 2024
@llucax llucax changed the title WIP Fix the condition to run the nox-cross-arch-all job May 3, 2024
@llucax llucax added the type:bug Something isn't working label May 3, 2024
@llucax llucax added this to the v0.10.0 milestone May 3, 2024
@llucax llucax self-assigned this May 3, 2024
@llucax llucax marked this pull request as ready for review May 3, 2024 12:59
@llucax llucax requested a review from a team as a code owner May 3, 2024 12:59
@llucax llucax added the cmd:skip-release-notes It is not necessary to update release notes for this PR label May 3, 2024
@llucax
Copy link
Contributor Author

llucax commented May 3, 2024

Skipping release notes because the previous release notes are enough.

@llucax llucax requested a review from Marenz May 3, 2024 13:00
@llucax
Copy link
Contributor Author

llucax commented May 3, 2024

I tested this quite a bit in this PR, simulation skips, failures and successes, I hope it is really working properly now.

@llucax llucax enabled auto-merge May 3, 2024 13:01
Marenz
Marenz previously approved these changes May 6, 2024
Signed-off-by: Leandro Lucarella <[email protected]>
The current condition makes the job skip unless there is a failure,
which means we also skip on success, which is not right, as we want this
job to succeed if the matrix job succeeds.

To fix this, we only skip if the `nox-cross-arch` matrix job was
skipped and we run otherwise, failing the job if the matrix job was not
successful. We also use the job name explicitly, as we only have one
dependency and it is easier to reason about a string than about arrays.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax llucax force-pushed the fix-cross-arch branch from 468b880 to d9566c8 Compare May 6, 2024 08:59
@llucax
Copy link
Contributor Author

llucax commented May 6, 2024

Fixed the quoting style in the if: condition (" is not supported) and also sneaked in a fix for a typo in labeler.yaml.

@llucax llucax enabled auto-merge May 6, 2024 08:59
@llucax llucax requested a review from Marenz May 6, 2024 08:59
Marenz
Marenz previously approved these changes May 6, 2024
Even when this is not necessary for now, as we are not skipping jobs in
the `nox` matrix job, it's better to have a consistent behavior to avoid
surprises in the future.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Copy link
Contributor Author

llucax commented May 6, 2024

Damn, one golden test was not properly updated. Now it is green, but news a new approval 🙏

@llucax llucax added this pull request to the merge queue May 6, 2024
Merged via the queue into frequenz-floss:v0.x.x with commit 21b8789 May 6, 2024
14 checks passed
@llucax llucax deleted the fix-cross-arch branch May 6, 2024 09:48
github-merge-queue bot pushed a commit to frequenz-floss/frequenz-client-base-python that referenced this pull request May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmd:skip-release-notes It is not necessary to update release notes for this PR part:ci Affects the GitHub workflow and other parts for running CI part:template Affects the cookiecutter template files type:bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants