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 repository name in CI workflow #904

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

glatterf42
Copy link
Member

@glatterf42 glatterf42 commented Jan 8, 2025

This PR aims to eventually fix how we run our CI workflows from forks. We want the tests to run only once and to be able to access our main repo's secrets if labelled correctly.

First, I'll not add any label to this PR, so it should not be able to access the secrets, which should make the tests fail.
Then I'll add the label and push something, which should trigger exactly one workflow: the "Receive PR" workflow should complete successfully, triggering one run of the tests with access to our secrets.

I'm not convinced this will work already because the trigger for pytest.yaml is also "on PR to main", so I'd have to look into combining this with "and not from within the head repo".

How to review

Look at the triggered CI runs and check they align with our intentions.

PR checklist

  • Continuous integration checks all ✅
  • Add or expand tests; coverage checks both ✅
  • [ ] Add, expand, or update documentation. Just CI.
  • [ ] Update release notes. Just CI.

@glatterf42 glatterf42 self-assigned this Jan 8, 2025
Copy link

codecov bot commented Jan 8, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.6%. Comparing base (a60715a) to head (dc27bb9).

Current head dc27bb9 differs from pull request most recent head c3dbee3

Please upload reports for the commit c3dbee3 to get more accurate results.

Additional details and impacted files
@@          Coverage Diff          @@
##            main    #904   +/-   ##
=====================================
  Coverage   95.6%   95.6%           
=====================================
  Files         46      46           
  Lines       4338    4338           
=====================================
  Hits        4148    4148           
  Misses       190     190           

@glatterf42
Copy link
Member Author

So the "Receive PR" workflow fails as expected since it's missing the label. However, I still triggered the "Pytest" workflow because the target is iiasa:main. This makes me think: the "Receive PR" workflow should be the only one with a trigger of "on PR to main". In this case, it would fail, but with a label or from within the IIASA repo, it should pass, triggering the "Pytest" workflow.

@khaeru
Copy link
Member

khaeru commented Jan 8, 2025

This makes me think: the "Receive PR" workflow should be the only one with a trigger of "on PR to main".

That seems like sound reasoning to me. Could you please try to make the change and see what happens?

@glatterf42
Copy link
Member Author

On it. What confuses me more is that the tests are actually passing, even though they should not have access to our secrets. Could this be because one doesn't need a license to access the cached GAMS installation?

@glatterf42
Copy link
Member Author

Pushing a commit without labeling the PR now triggers only the "Receive PR", "license/cla", and "Build package" tests.

@glatterf42
Copy link
Member Author

glatterf42 commented Jan 8, 2025

Adding the label gets "Receive PR" to pass, but the view of the tests here does not show that any of the other tests would be running now.

The commit I used to trigger the update was the improvement @khaeru suggested here.

EDIT: the test may be running, though, at least the RTD build passed now. Usually, Code quality should have passed already.

@glatterf42
Copy link
Member Author

glatterf42 commented Jan 8, 2025

Maybe these statuses are only reported back once the whole workflow finishes?
But even that seems to have happened already: https://github.com/iiasa/message_ix/actions/runs/12668365243/job/35303644902
It looks to me like the "Receive PR" workflow triggers the "Pytest" workflow as it is on IIASA:main, which still fails due to github.event.workflow_run.repository being an object (where a string is expected by the checkout action).

So I think we should make a PR from within the IIASA repo to add the first two commits from this branch, then rebase this PR to see if it works as expected now.

@glatterf42 glatterf42 mentioned this pull request Jan 8, 2025
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants