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

Don't run scheduled jobs on forks #584

Merged
merged 3 commits into from
Mar 18, 2021
Merged

Don't run scheduled jobs on forks #584

merged 3 commits into from
Mar 18, 2021

Conversation

christophebedard
Copy link
Member

@christophebedard christophebedard commented Mar 16, 2021

I often enable actions on my fork to open PRs (on my own fork) to test things out with CI. There's no real reason to run hourly scheduled jobs on forks.

I tried these and they didn't work:

  • if: ${{ (github.event_name != 'schedule') || (github.event_name == 'schedule' && !github.event.repository.fork) }}
  • if: ${{ (github.event_name != 'schedule') || (github.event_name == 'schedule' && github.event.repository.fork == false) }}

@christophebedard christophebedard requested a review from a team as a code owner March 16, 2021 13:51
@christophebedard christophebedard requested review from Karsten1987 and jaisontj and removed request for a team March 16, 2021 13:51
@christophebedard
Copy link
Member Author

Well this doesn't seem to work. I'll try something else.

@christophebedard christophebedard marked this pull request as draft March 16, 2021 15:09
@codecov
Copy link

codecov bot commented Mar 16, 2021

Codecov Report

Merging #584 (179c4b8) into master (c5b726b) will decrease coverage by 0.93%.
The diff coverage is n/a.

❗ Current head 179c4b8 differs from pull request most recent head d74cd11. Consider uploading reports for the commit d74cd11 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #584      +/-   ##
==========================================
- Coverage   39.90%   38.96%   -0.94%     
==========================================
  Files           2        2              
  Lines         213      213              
  Branches       39       39              
==========================================
- Hits           85       83       -2     
- Misses        128      130       +2     
Impacted Files Coverage Δ
src/action-ros-ci.ts 29.34% <0.00%> (-1.09%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c5b726b...d74cd11. Read the comment docs.

Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
@christophebedard christophebedard marked this pull request as ready for review March 16, 2021 20:10
@christophebedard
Copy link
Member Author

It works now.

@emersonknapp
Copy link
Contributor

Is there any way that we can reduce the duplication?

@christophebedard
Copy link
Member Author

Is there any way that we can reduce the duplication?

I'll try defaults.if.

@christophebedard
Copy link
Member Author

I'll try defaults.if.

It's not valid apparently:

defaults:
  # Don't run scheduled jobs on forks
  if: ${{ (github.event_name != 'schedule') || (github.event_name == 'schedule' && github.repository == 'ros-tooling/action-ros-ci') }}
The workflow is not valid. .github/workflows/test.yml (Line: 14, Col: 3): Unexpected value 'if'

I can think of two alternatives, but they all require some level of duplication and are a bit more complex, so I'm not sure if it's worth it:

  1. have a top-level env: using the big condition, and have a simple if: for each job (not sure if that works though)
  2. define an empty/trivial job (which uses the big if: condition) and have all other jobs depend on it using needs: so that they only run if the first job runs

I'll try option 2.

Note that YAML anchors are unfortunately not supported: https://github.community/t/support-for-yaml-anchors/16128/61

Signed-off-by: Christophe Bedard <bedard.christophe@gmail.com>
@christophebedard
Copy link
Member Author

Copy link
Contributor

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

Thanks for figuring out this way to do it - it looks pretty clean to me

@emersonknapp emersonknapp enabled auto-merge (squash) March 17, 2021 20:39
@emersonknapp emersonknapp merged commit 987e5c6 into ros-tooling:master Mar 18, 2021
@christophebedard christophebedard deleted the skip-scheduled-jobs-on-forks branch March 18, 2021 12:03
@dhimmel
Copy link

dhimmel commented Dec 2, 2021

if: ${{ (github.event_name != 'schedule') || (github.event_name == 'schedule' && !github.event.repository.fork) }}... Well this doesn't seem to work. I'll try something else.

Noting that the reason this didn't work is because the github.event.repository.fork context is not set for scheduled jobs.

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

Successfully merging this pull request may close these issues.

None yet

3 participants