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.

Sign up for GitHub

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
+0 −1
Merged

fix(ci): Daily workflows report #1200

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c7d13f3
Fix daily workflows getting green tick when jobs failed.
AlejandroCabeza Sep 24, 2024
1862b7d
Add pull_request trigger to daily workflows to check they work during…
AlejandroCabeza Sep 25, 2024
acb7b59
Rollback to continue-on-error to compare.
AlejandroCabeza Sep 27, 2024
7df8b35
Remove continue-on-error line.
AlejandroCabeza Oct 1, 2024
bfa8436
cleanup
diegomrsantos Oct 10, 2024
a426ccc
Merge branch 'master' into fix/issue-1197/wrong-daily-workflow-reports
diegomrsantos Oct 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/daily_common.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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?

steps:
- name: Checkout
uses: actions/checkout@v4
Expand Down
Loading