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

Prevent tests from running on forks #2280

Merged
merged 3 commits into from
Apr 27, 2023

Conversation

atharva-2001
Copy link
Member

📝 Description

This is to prevent consumption of LFS resources.

📌 Resources

Examples, notebooks, and links to useful references.
https://docs.github.com/en/actions/learn-github-actions/contexts#github-context
https://github.com/atharva-2001/tardis/actions/runs/4742893534

🚦 Testing

How did you test these changes?

  • Testing pipeline
  • Other method (describe)
  • My changes can't be tested (explain why)

☑️ Checklist

  • I requested two reviewers for this pull request
  • I updated the documentation according to my changes
  • I built the documentation by applying the build_docs label

Note: If you are not allowed to perform any of these actions, ping (@) a contributor.

@andrewfullard
Copy link
Contributor

I take it forks cannot use caches in the same way? And even if they can, they will still have to generate a cache to begin with?

@codecov
Copy link

codecov bot commented Apr 19, 2023

Codecov Report

Merging #2280 (53a1ace) into master (effacfb) will decrease coverage by 0.03%.
The diff coverage is 60.60%.

❗ Current head 53a1ace differs from pull request most recent head 8351415. Consider uploading reports for the commit 8351415 to get more accurate results

@@            Coverage Diff             @@
##           master    #2280      +/-   ##
==========================================
- Coverage   71.84%   71.81%   -0.03%     
==========================================
  Files         133      135       +2     
  Lines       12398    12431      +33     
==========================================
+ Hits         8907     8927      +20     
- Misses       3491     3504      +13     
Impacted Files Coverage Δ
.../montecarlo/montecarlo_numba/nonhomologous_grid.py 48.00% <48.00%> (ø)
tardis/montecarlo/tests/test_nonhomologous.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

andrewfullard
andrewfullard previously approved these changes Apr 21, 2023
@atharva-2001
Copy link
Member Author

concurrency:
group: ${{ github.workflow }}-${{ github.head_ref || github.ref }}
cancel-in-progress: true

I'm afraid I don't know if this works completely as intended.
The workflow should run only once for a commit, right?
${{ github.ref }} is different for forks and for upstream repo.
https://github.com/atharva-2001/tardis/actions/runs/4798029406/jobs/8535766612
https://github.com/tardis-sn/tardis/actions/runs/4798029889/jobs/8535760860

Here is the workflow file.
https://github.com/tardis-sn/tardis/actions/runs/4798029889/workflow

@epassaro
Copy link
Member

epassaro commented Apr 25, 2023

concurrency:
group: ${{ github.workflow }}-${{ github.head_ref || github.ref }}
cancel-in-progress: true

I'm afraid I don't know if this works completely as intended. The workflow should run only once for a commit, right? ${{ github.ref }} is different for forks and for upstream repo. https://github.com/atharva-2001/tardis/actions/runs/4798029406/jobs/8535766612 https://github.com/tardis-sn/tardis/actions/runs/4798029889/jobs/8535760860

Here is the workflow file. https://github.com/tardis-sn/tardis/actions/runs/4798029889/workflow

As far as I remember, that creates a group depending if the workflow runs on a branch of the repository or runs on a pull request.

The part ${{ github.head_ref || github.ref }} means, if github.head_ref does not exist fallback to github.ref. One of these variables exist if the workflow runs in a branch of the repo, the other exist in pull requests (or exists in both scenarios?, I don't remember).

It wasn't designed with forks in mind, I don't know what happens there.

EDIT: I think it's working as expected, you have different results because one run occurs in a branch of your fork, and the other in a pull request.

@atharva-2001
Copy link
Member Author

I agree- this is working correctly. My apologies. I also found this- might be of interest:
https://stackoverflow.com/a/75403978/11974464

@andrewfullard andrewfullard enabled auto-merge (squash) April 26, 2023 13:19
@andrewfullard andrewfullard merged commit f79c6d1 into tardis-sn:master Apr 27, 2023
light2802 pushed a commit to light2802/tardis that referenced this pull request May 27, 2023
* set repository owner

* has to be in quotes

* Leave spaces before and after ==
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants