-
Notifications
You must be signed in to change notification settings - Fork 612
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
infra: improve tox for local development #2999
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: emdneto <[email protected]>
Signed-off-by: emdneto <[email protected]>
Signed-off-by: emdneto <[email protected]>
Signed-off-by: emdneto <[email protected]>
@@ -388,450 +388,278 @@ envlist = | |||
ruff | |||
|
|||
[testenv] | |||
test_deps = |
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.
Group common packages here. Save some lines in the file.
opentelemetry-instrumentation: pip install opentelemetry-sdk@{env:CORE_REPO}\#egg=opentelemetry-sdk&subdirectory=opentelemetry-sdk | ||
opentelemetry-instrumentation: pip install opentelemetry-test-utils@{env:CORE_REPO}\#egg=opentelemetry-test-utils&subdirectory=tests/opentelemetry-test-utils | ||
opentelemetry-instrumentation: pip install -r {toxinidir}/opentelemetry-instrumentation/test-requirements.txt | ||
distro: {[testenv]test_deps} |
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.
Here is how we are installing common packages now
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.
removed this from tox.ini and created a requirement.txt file.
The cons are that it is more susceptible to dependabot alerts. Should we keep tox.ini, or is it okay to use a requirement.txt file for docker tests since there are a lot of dependencies?
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.
I think it's ok but we'll need to take care of dependabots :)
Signed-off-by: emdneto <[email protected]>
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.
I didn't update [testenv:docs]
because it's failing: https://github.com/open-telemetry/opentelemetry-python-contrib/actions/runs/11827704954/job/32956450432
One solution would be separate docs-requirements.txt in two requirements-files (docs-requirements.txt and readthedocs.txt and use the appropriate one for .readthedocs.yml
.
Updated docs-requirements.txt to not use editable installs pypa/pip#12502
Signed-off-by: emdneto <[email protected]>
-e "git+https://github.com/open-telemetry/opentelemetry-python.git#egg=opentelemetry-sdk&subdirectory=opentelemetry-sdk" | ||
-e "git+https://github.com/open-telemetry/opentelemetry-python-contrib.git#egg=opentelemetry-util-http&subdirectory=util/opentelemetry-util-http" | ||
./opentelemetry-instrumentation | ||
# doesn't work for pkg_resources. Used by .readthedocs.yml |
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.
Is the pkg_resources hunk still relevant?
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.
I'm not sure about the comment itself but, I'm pretty sure we still need install core packages to run in readthedocs
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.
Yeah I meant just the comment 😅
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.
Not a tox expert but it simplifies tox.ini and removes 200 lines so LGTM
Description
Same as open-telemetry/opentelemetry-python#4238