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

infra: improve tox for local development #2999

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

Conversation

emdneto
Copy link
Member

@emdneto emdneto commented Nov 13, 2024

@emdneto emdneto requested a review from a team as a code owner November 13, 2024 23:04
Signed-off-by: emdneto <[email protected]>
@emdneto emdneto marked this pull request as draft November 14, 2024 00:21
Signed-off-by: emdneto <[email protected]>
@@ -388,450 +388,278 @@ envlist =
ruff

[testenv]
test_deps =
Copy link
Member Author

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}
Copy link
Member Author

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

Copy link
Member Author

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?

Copy link
Contributor

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]>
@emdneto emdneto marked this pull request as ready for review November 14, 2024 01:11
@emdneto emdneto added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Nov 14, 2024
Copy link
Member Author

@emdneto emdneto left a 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
Copy link
Contributor

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?

Copy link
Member Author

@emdneto emdneto Nov 14, 2024

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

Copy link
Contributor

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 😅

Copy link
Contributor

@xrmx xrmx left a 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build & infra Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants