fix (ci): Allow opentelemetry-python forks to use CI #3190
Conversation
Signed-off-by: Josh Soref <[email protected]>
| CORE_REPO_SHA: main | ||
| CONTRIB_REPO: open-telemetry/opentelemetry-python-contrib |
There was a problem hiding this comment.
Does this need to be${{ github.repository }} so users can run tests of contrib forks too? Just going off of symmetry here
There was a problem hiding this comment.
Sorry, i can't think through what it should be at this time. If i have time/energy when I'm not at work, I can try to reason it out.
what I put here is almost certainly wrong, but the various templates and indirections are making me way too dizzy.
There was a problem hiding this comment.
@aabmass I think this is not needed, because checkout action already does the trick for us, including forks.
Example In my fork: https://github.com/emdneto/opentelemetry-python-contrib/actions/runs/12788418223/job/35649787442
This variable has no use though
There was a problem hiding this comment.
In principle, I believe, this could be needed if someone was trying to test a change to their opentelemetry-python repository that needed changes to their opentelemetry-python-contrib repository at the same time (which is more or less what we're looking at in open-telemetry/opentelemetry-python#4388 where things conceptually didn't work for https://github.com/jsoref/opentelemetry-python/actions/runs/12793563878 because the callee doesn't yet have the required feature).
I'm not saying it's working, but I suspect a thing like it would be needed. I'm also not entirely certain it would always work/would work in this case (I think you can't really programmatically change a called workflow target, so in this specific case it wouldn't, but for a normal case, of code as opposed to workflow dependencies, I think it'd be useful).
| @@ -682,10 +682,11 @@ allowlist_externals = | |||
| sh | |||
|
|
|||
| setenv = | |||
| ; override CORE_REPO via env variable when testing in forks | |||
| CORE_REPO=git+https://github.com/{env:CORE_REPO:open-telemetry/opentelemetry-python}.git@{env:CORE_REPO_SHA} | |||
There was a problem hiding this comment.
I would say to have less intrusive changes to CI, maybe just use the GITHUB_REPOSITORY runtime environment variable here? Like
| CORE_REPO=git+https://github.com/{env:CORE_REPO:open-telemetry/opentelemetry-python}.git@{env:CORE_REPO_SHA} | |
| CORE_REPO=git+https://github.com/{env:GITHUB_REPOSITORY:open-telemetry/opentelemetry-python}.git@{env:CORE_REPO_SHA} |
At CI this will point to the current owner and repository name, so when running in your fork, it should match your check-spelling-sandbox/opentelemetry-python. This will make tox look at your fork+revision correctly
|
|
||
| ```sh | ||
| CORE_REPO=check-spelling-sandbox/opentelemetry-python |
There was a problem hiding this comment.
| CORE_REPO=check-spelling-sandbox/opentelemetry-python | |
| GITHUB_REPOSITORY=your-fork/opentelemetry-python tox |
Based on my previous comment
Description
Adds
CORE_REPOandCONTRIB_REPOinputs and corresponding environment variables to allow people to use CI in forks of https://github.com/open-telemetry/opentelemetry-pythonType of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Does This PR Require a Core Repo Change?
There is a corresponding change that should be made but it it can be done after this is merged.
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.