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(ci): Daily workflows report #1200

Merged
merged 6 commits into from
Oct 10, 2024

Conversation

AlejandroCabeza
Copy link
Collaborator

@AlejandroCabeza AlejandroCabeza commented Sep 24, 2024

Fix daily workflows' green tick when jobs failed.

Based of off: https://stackoverflow.com/a/58859404

Closes: #1197

@@ -60,7 +60,7 @@ jobs:

name: '${{ matrix.platform.os }}-${{ matrix.cpu }} (Nim ${{ matrix.nim.branch }})'
runs-on: ${{ matrix.platform.builder }}
continue-on-error: ${{ matrix.nim.branch == 'devel' || matrix.nim.branch == 'version-2-0' }}
if: success() || failure() # Continue jobs even if previous jobs failed, but not if cancelled.
Copy link
Contributor

@lchenut lchenut Sep 25, 2024

Choose a reason for hiding this comment

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

I'm not 100% sure this change will fix the problem. I just see that as a way to write continue-on-error differently. But I dig into this a bit too much and I think removing this line will solve the issue.
With fail-fast: false defined in the strategy up above, the jobs will not stop the whole pipeline on failure, even without this line (or the continue-on-error).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm also not completely sure. But according to the post I mentioned in the description it apparently will do what it suggests.

I could set the daily jobs to run on PR so we can get the results here, now that I think about it.

Copy link
Collaborator Author

@AlejandroCabeza AlejandroCabeza Sep 25, 2024

Choose a reason for hiding this comment

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

It seems it worked, as the jobs have been marked as failed? I can't say for sure, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you try without this line, just for my own sanity? 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Besides that, like you said, it seems it worked. Just remove the on pull_request and I approve it.

Copy link
Collaborator Author

@AlejandroCabeza AlejandroCabeza Sep 27, 2024

Choose a reason for hiding this comment

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

The test runs finished a bit ago: Even though here the checks show as failed (with continue-on-error), if you check the Actions tab you'll see a difference between them. The workflows run with continue-on-error show as green tick (even if they failed), while the ones with the if: success() || failure() show as failed; which is the intended behaviour.

Copy link
Contributor

@lchenut lchenut Oct 1, 2024

Choose a reason for hiding this comment

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

I meant without continue-on-error and without if: success() || failure(). Sorry if I didn't make myself clear.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, I see. Doing that :)

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems to work without it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

True. Shall we go for this?

@diegomrsantos diegomrsantos added this pull request to the merge queue Oct 10, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 10, 2024
@diegomrsantos diegomrsantos added this pull request to the merge queue Oct 10, 2024
Merged via the queue into master with commit a3b8729 Oct 10, 2024
14 checks passed
@diegomrsantos diegomrsantos deleted the fix/issue-1197/wrong-daily-workflow-reports branch October 10, 2024 12:58
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.

Daily jobs report success even when workflows failed
3 participants