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
upgrade plux to 1.7 #10266
upgrade plux to 1.7 #10266
Conversation
S3 Image Test Results (AMD64 / ARM64) 2 files 2 suites 3m 11s ⏱️ Results for commit 666b76e. ♻️ This comment has been updated with latest results. |
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.
The changes are looking good! The release implements a nice deprecation path, and can seamlessly be upgraded. 🔝
I only found a small nitpick, and I think it would be a good opportunity to discuss upper version limits in the setup.cfg
.
setup.cfg
Outdated
@@ -31,7 +31,7 @@ install_requires = | |||
dill==0.3.6 | |||
dnslib>=0.9.10 | |||
dnspython>=1.16.0 | |||
plux>=1.3.1 | |||
plux>=1.7,<2.0 |
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 this is a good opportunity to discuss upper version limits.
We explicitly removed pins like this in #10195.
We did this because there's no need for the pins for as long as the currently released version is compatible.
New releases will either be picked up proactively by us (in PRs like this, explicitly upgrading a single dependency), or they are picked up by the weekly upgrades where the breaking changes are identified and (a) fixed in the PR by adding a small fix, or (b) pinned and explicitly upgraded in a follow up PR (where the min version is updated and the upper version limit removed).
In this case, the library is maintained by us, meaning that we already explicitly know that 2.0
will contain breaking changes. However, if the person who creates the release does not proactively update it here, we most likely won't find out (because the automated updates won't dare to touch it 😛)...
This is why I am leaning towards not having these pins, but again, this is a great opportunity to discuss them. 😄
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.
Sorry for the initial "comment" instead of the approval, that was a mistake.
I am happy with merging this as it is, but I would be happy to discuss the comment concerning the upper version limit. 😄
FYI: Due to the nice deprecation path / backwards compatibility of the changes in |
Co-authored-by: Alexander Rashed <2796604+alexrashed@users.noreply.github.com>
Motivation
This PR upgrade plux to 1.7 which provides python 3.12 support (see #10235). The PR also migrates
plugin
imports to the new way to do things (from plux import ...
). Since 1.7 is backwards compatible, we can make these changes for ext separately.Moreover, plux now has a way of regenerating entrypoints without invoking setup.py (which is deprecated), therefore unblocking us from moving away from a
setup.py
topython -m build
.We could also look into removing
localstack.cli.plugins
, and move some of the functionality to plux (if needed).Changes
make entrypoints
now uses the plux frontendpython -m plux entrypoints
instead ofsetup.py plugins egg_info
from plux
for imports instead offrom plugin