-
Notifications
You must be signed in to change notification settings - Fork 128
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
Use looseversion in place of distutils #904
base: stable/caracal
Are you sure you want to change the base?
Conversation
(cherry picked from commit 012b2d6)
Blocker for s390x validation |
@@ -12,3 +12,5 @@ Jinja2 | |||
netaddr | |||
|
|||
pbr!=2.1.0,>=2.0.0 # Apache-2.0 | |||
|
|||
looseversion;python_version >= '3.12' |
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.
how does this work when being used in a classic charm?
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.
Based on your comment above, it probably breaks unless the parent charm ensures that the package is installed.
try: | ||
from distutils.version import LooseVersion | ||
except ImportError: | ||
from looseversion import LooseVersion |
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.
shouldn't this be more like:
try:
from distutils.version import LooseVersion
except ImportError:
try:
# reactive charm path, where the venv already has this package
from looseversion import LooseVersion
except ImportError:
# classic charm code path where python packages are installed via apt-get
apt_install(['python3-looseversion'], fatal=True)
from looseversion import LooseVersion
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 really don't like this pattern where charmhelpers runs apt_install
; I'd much rather the parent charm resolved the dependencies and installed python3-looseversion where necessary; (which might be all of them in the future). It makes unit testing harder when apt_install()
is at module load time.
try: | ||
from distutils.version import LooseVersion | ||
except ImportError: | ||
from looseversion import LooseVersion |
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 really don't like this pattern where charmhelpers runs apt_install
; I'd much rather the parent charm resolved the dependencies and installed python3-looseversion where necessary; (which might be all of them in the future). It makes unit testing harder when apt_install()
is at module load time.
@@ -12,3 +12,5 @@ Jinja2 | |||
netaddr | |||
|
|||
pbr!=2.1.0,>=2.0.0 # Apache-2.0 | |||
|
|||
looseversion;python_version >= '3.12' |
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.
Based on your comment above, it probably breaks unless the parent charm ensures that the package is installed.
Holding off merging whilst @freyes conversation is concluded; there may be further changes needed to the original commits. |
(cherry picked from commit 012b2d6)