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

Put the xray propagator’s test dep. on requests in the test extra, not just tox.ini #2167

Closed

Conversation

musicinmybrain
Copy link
Contributor

Description

The opentelemetry-propagator-aws-xray package uses requests in its tests,

but lacks a direct dependency on it in the tests extra:

This has probably not been noticed because there are already test dependencies on requests elsewhere in the repository, e.g. in opentelemetry-instrumentation-fastapi and in opentelemetry-exporter-prometheus-remote-write, so it ends up installed when testing the repository as a whole.

This PR simply adds the missing dependency.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

This was considered too trivial to test.

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated N/A
  • Unit tests have been added N/A
  • Documentation has been updated N/A

@musicinmybrain
Copy link
Contributor Author

I see now that the dependency is also handled in tox.ini,

propagator-aws-xray: pip install requests {toxinidir}/propagator/opentelemetry-propagator-aws-xray[test]

but I don’t see any reason it shouldn’t appear in the test extra. Instead, I’m pushing an extra commit that would remove the manual pip install requests from tox.ini.

@musicinmybrain musicinmybrain changed the title Add missing test dep. on requests for xray propagator Put the xray propagator’s test dep. on requests in the test extra, not just tox.ini Feb 11, 2024
@xrmx
Copy link
Contributor

xrmx commented Mar 8, 2024

Will this PR still be required after #2320 ?

@musicinmybrain
Copy link
Contributor Author

Will this PR still be required after #2320 ?

No, if the test extras are going away, as in #2320, then this PR makes no sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants