-
Notifications
You must be signed in to change notification settings - Fork 642
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 (ci): Allow opentelemetry-python forks to use CI #3190
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -242,9 +242,12 @@ tox -e py312-test-instrumentation-aiopg | |||||
|
||||||
### Testing against a different Core repo branch/commit | ||||||
|
||||||
Some of the tox targets install packages from the [OpenTelemetry Python Core Repository](https://github.com/open-telemetry/opentelemetry-python) via pip. The version of the packages installed defaults to the main branch in that repository when tox is run locally. It is possible to install packages tagged with a specific git commit hash by setting an environment variable before running tox as per the following example: | ||||||
Some of the tox targets install packages from the [OpenTelemetry Python Core Repository](https://github.com/open-telemetry/opentelemetry-python) via pip. | ||||||
The version of the packages installed defaults to the main branch in that repository when tox is run locally. | ||||||
It is possible to install packages tagged with a specific git commit hash (optionally in a specific fork) by setting these environment variables before running tox as per the following example: | ||||||
|
||||||
```sh | ||||||
CORE_REPO=check-spelling-sandbox/opentelemetry-python | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Based on my previous comment |
||||||
CORE_REPO_SHA=c49ad57bfe35cfc69bfa863d74058ca9bec55fc3 tox | ||||||
``` | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would say to have less intrusive changes to CI, maybe just use the
Suggested change
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 |
||||||
; override CORE_REPO_SHA via env variable when testing other branches/commits than main | ||||||
; i.e: CORE_REPO_SHA=dde62cebffe519c35875af6d06fae053b3be65ec tox -e <env to test> | ||||||
CORE_REPO_SHA={env:CORE_REPO_SHA:main} | ||||||
CORE_REPO=git+https://github.com/open-telemetry/opentelemetry-python.git@{env:CORE_REPO_SHA} | ||||||
|
||||||
commands_pre = | ||||||
; In order to get a health coverage report, | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be
${{ github.repository }}
so users can run tests of contrib forks too? Just going off of symmetry hereThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).