Skip to content

CI: split PR workflows, add paths #708

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

Jump to bottom
Closed
wants to merge 24 commits into from
Closed

CI: split PR workflows, add paths #708

wants to merge 24 commits into from

Conversation

markgoddard
Copy link
Contributor

@markgoddard markgoddard commented Oct 12, 2023 •
edited
Loading

  • CI: Split .github/workflows/stackhpc-pull-request.yml into tox and aio workflows
  • CI: Add paths to stackhpc-pull-request-tox-aio.yml workflow

This should reduce our CI overheads by only running aios when necessary. It will also make it easier to merge some changes.

@markgoddard markgoddard requested a review from a team as a code owner October 12, 2023 09:41
@markgoddard markgoddard self-assigned this Oct 12, 2023
@markgoddard
Copy link
Contributor Author

Cancelled CI jobs because we can see they were triggered correctly.

Alex-Welsh
Alex-Welsh reviewed Oct 12, 2023
View reviewed changes
@markgoddard
Copy link
Contributor Author

I suppose the difficulty is that we wouldn't be able to make the aio jobs required any more :(

Oh GH actions, you are no zuul.

@Alex-Welsh
Copy link
Member

I suppose the difficulty is that we wouldn't be able to make the aio jobs required any more :(

Oh GH actions, you are no zuul.

Can we fake running it and immediately return a success?

@markgoddard
Copy link
Contributor Author

I suppose the difficulty is that we wouldn't be able to make the aio jobs required any more :(
Oh GH actions, you are no zuul.

Can we fake running it and immediately return a success?

Possibly, but it would be a bit misleading and would need something other than the paths selector

@markgoddard
CI: Skip aio jobs based on changed files
978a5b3 
We don't always want to run all aio jobs - it can be wasteful of CI
resources and slows down merging changes.

This change uses the dorny/paths-filter action to detect changed files
and skip jobs as appropriate.

Note that we can't use the workflow-level paths attribute since this
would skip the workflow entirely, and would prevent us from making the
aio jobs required to pass (a skip counts as a pass).
@markgoddard markgoddard force-pushed the wallaby-split-ci-workflows branch 3 times, most recently from 797095b to cb63995 Compare October 13, 2023 08:57
@markgoddard
CI: Remove unnecessary conditionals
2b4e221 
The job dependency should handle this, with the condition applied to the first job.
@markgoddard markgoddard force-pushed the wallaby-split-ci-workflows branch 2 times, most recently from 3f1545b to 2b4e221 Compare October 13, 2023 09:17
@Alex-Welsh
Copy link
Member

How did the testing for this go? does it need more work?

mnasiadka
mnasiadka previously approved these changes Oct 25, 2023
View reviewed changes
@markgoddard
Copy link
Contributor Author

How did the testing for this go? does it need more work?

Looks like it doesn't accept the skipped jobs :(

@jovial
Copy link
Contributor

jovial commented Oct 26, 2023 •
edited
Loading

How did the testing for this go? does it need more work?

Looks like it doesn't accept the skipped jobs :(

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/collaborating-on-repositories-with-code-quality-features/troubleshooting-required-status-checks#handling-skipped-but-required-checks. Seems like the only option is skip all jobs within a workflow rather than the workflow itself :(

@markgoddard
Copy link
Contributor Author

How did the testing for this go? does it need more work?

Looks like it doesn't accept the skipped jobs :(

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/collaborating-on-repositories-with-code-quality-features/troubleshooting-required-status-checks#handling-skipped-but-required-checks. Seems like the only option is skip all jobs within a workflow rather than the workflow itself :(

That's what it's doing already. I was relying on job dependencies though and have just tried adding the condition to every job. Could you approve again?

It does look like it's just not recognising the jobs though as it lists them as skipped and expected.

@markgoddard markgoddard requested a review from mnasiadka October 26, 2023 13:47
jovial
jovial previously approved these changes Oct 26, 2023
View reviewed changes
@markgoddard
Copy link
Contributor Author

I managed to tweak the branch protection rules to require the calling workflow's job rather than the called workflow, and this seems to now recognise the skipped results as success. I guess it never gets to the called workflow if the calling job is skipped.

Just need to confirm that this still requires the jobs to pass when they run 🤞

@markgoddard
Copy link
Contributor Author

I managed to tweak the branch protection rules to require the calling workflow's job rather than the called workflow, and this seems to now recognise the skipped results as success. I guess it never gets to the called workflow if the calling job is skipped.

Just need to confirm that this still requires the jobs to pass when they run 🤞

Lame, it doesn't work:

image

I'll revert the required status checks change.

@markgoddard markgoddard force-pushed the wallaby-split-ci-workflows branch 2 times, most recently from 83439bb to a83be10 Compare October 27, 2023 07:59
@markgoddard
Copy link
Contributor Author

No changed files so jobs not triggered
No always()
Status not recognised.

@markgoddard
Copy link
Contributor Author

No changed files so jobs not triggered
Using always()
Status recognised.

@markgoddard
Copy link
Contributor Author

Changed file triggers jobs
Working build kayobe image job
Using always()
Status recognised.

@markgoddard
Copy link
Contributor Author

Changed file triggers jobs
Failing build kayobe image job
Using always()
Status recognised.
aio jobs run but should be skipped

@markgoddard markgoddard force-pushed the wallaby-split-ci-workflows branch 4 times, most recently from e0b0634 to 065fd29 Compare October 27, 2023 18:48
@markgoddard markgoddard force-pushed the wallaby-split-ci-workflows branch from 065fd29 to cd92fb2 Compare October 27, 2023 18:50
@markgoddard
Copy link
Contributor Author

No changed files so jobs not triggered
Using ${{ ! failure() }}
Status recognised.

Using always() && needs.build-kayobe-image.result == 'success'
Status not recognised: result == 'skipped'.

@markgoddard
Copy link
Contributor Author

markgoddard commented Oct 27, 2023 •
edited
Loading

No changed files so jobs not triggered
Using always() && needs.build-kayobe-image.result != 'failure'
Or using ${{ ! failure() }}
Status recognised.

@markgoddard
Copy link
Contributor Author

markgoddard commented Oct 27, 2023 •
edited
Loading

Changed file triggers jobs
Failing build kayobe image job
Using always() && needs.build-kayobe-image.result != 'failure'
Or using ${{ ! failure() }}
aio jobs skipped & not recognised as success

@markgoddard
Copy link
Contributor Author

markgoddard commented Oct 27, 2023 •
edited
Loading

Changed file triggers jobs
Working build kayobe image job
Using always() && needs.build-kayobe-image.result != 'failure'
Or using ${{ ! failure() }}
aio jobs run & recognised as success

@markgoddard
Copy link
Contributor Author

Looks like either of these should do the trick

@markgoddard
Copy link
Contributor Author

This PR is a bit of a mess. Closing in favour of #726

@markgoddard markgoddard mentioned this pull request Oct 30, 2023
Merged
@markgoddard markgoddard deleted the wallaby-split-ci-workflows branch February 16, 2024 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Alex-Welsh Alex-Welsh Alex-Welsh left review comments

@jovial jovial jovial left review comments

@mnasiadka mnasiadka Awaiting requested review from mnasiadka

Assignees

@markgoddard markgoddard

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@markgoddard @Alex-Welsh @jovial @mnasiadka