search

PMExtra / sentry-auth-ldap

A Sentry extension to add an LDAP server as an authention source.
Apache License 2.0
27 stars 7 forks source link

Avoid explicitly depending on Sentry by default #7

Closed Asgavar closed 1 year ago

Asgavar commented 1 year ago

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 Hub^1.

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
PMExtra commented 1 year ago

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 commented 1 year ago

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

Asgavar commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

PMExtra commented 1 year ago

I think it's a limitation of Poetry. https://github.com/python-poetry/poetry/issues/697

PMExtra commented 1 year ago

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

Asgavar commented 1 year ago

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