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

Avoid explicitly depending on Sentry by default #7

Closed
wants to merge 1 commit into from

Conversation

Asgavar
Copy link

@Asgavar Asgavar commented May 25, 2023

As this module is more or less a plugin to Sentry proper, it seems extremely unlikely that this dependency is actually used - most users would probably already have Sentry and installing sentry-auth-ldap alongside.

Keeping it here, however, makes using things like Pipenv/Poetry/etc a bit inconvenient as the newest (>= 21.9.0) version of Sentry (plus a lot of its subdependencies) will always end up in the lockfile - and Sentry itself may have come from another source, external to the package manager, such as the getsentry/sentry image from Docker Hub1.

After this commit, it will still be possible to pull Sentry into the dependency tree after e.g.:

-sentry-auth-ldap==21.9.11
+sentry-auth-ldap[sentry]==21.9.11

Footnotes

  1. https://hub.docker.com/r/getsentry/sentry

As this module is more or less a plugin to Sentry proper, it seems extremely
unlikely that this dependency is actually used - most users would probably
already have Sentry and install sentry-auth-ldap alongside.

Keeping it here, however, makes using things like Pipenv/Poetry/etc a bit
inconvenient as the newest (>= 21.9.0) version of Sentry (plus _a lot_ of
its subdependencies) will always end up in the lockfile - and Sentry itself
may have come from another source, external to the package manager, such as
the `getsentry/sentry` image from Docker Hub[^1].

After this commit, it will still be possible to pull Sentry into the
dependency tree after specifying it as an extra, e.g.:

```diff
-sentry-auth-ldap==21.9.11
+sentry-auth-ldap[sentry]==21.9.11
```

[^1]: https://hub.docker.com/r/getsentry/sentry
@PMExtra
Copy link
Owner

PMExtra commented May 25, 2023

I think the Sentry docker image is also included Sentry in site-packages.

The install_requires is used to constraint the version of Sentry

@PMExtra
Copy link
Owner

PMExtra commented May 25, 2023

It seems like extras_require is not designed for this usage case.

@Asgavar
Copy link
Author

Asgavar commented May 25, 2023

And thanks to Sentry being in site-packages, a simple pip install sentry-auth-ldap will not override it as long as it's >=21.9.0; Poetry's lockfiles, however, are supposed to be independent from the machine they're created on so will evaluate >=21.9.0 as the only constraint and thus arrive at the currently newest version - trying to install it even when Sentry is in site-packages already, cause the lockfile will now specify the $CURRENTLY_NEWEST version.

As for extras_require - I can also drop it completely if you think it's better.

@PMExtra
Copy link
Owner

PMExtra commented May 26, 2023

It seems like you should ignore the poetry.lock file between independent machines.
I don't think we should drop the Sentry dependency from install_requires.

@Asgavar
Copy link
Author

Asgavar commented May 26, 2023

If I ignore poetry.lock than the entire reason of using Poetry (and hashes generated by it) is invalidated

@PMExtra
Copy link
Owner

PMExtra commented May 26, 2023

I think it's a limitation of Poetry.
python-poetry/poetry#697

@PMExtra
Copy link
Owner

PMExtra commented May 26, 2023

The wonderful solution is something like peerDependencies of nodeJS (npm).
But Python / Poetry did not have such a thing.

@Asgavar
Copy link
Author

Asgavar commented May 26, 2023

Yeah, that would be ideal. OK though, if you feel like keeping it I'll close the PR.

@Asgavar Asgavar closed this May 26, 2023
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.

2 participants