Use warnings.warn instead of self.log.warning to help avoid duplications #133
Conversation
systemdspawner/systemdspawner.py
Outdated
| self.log.warning( | ||
| "Failed to parse systemd version from 'systemctl --version'" | ||
| ) | ||
| warnings.warn("Failed to parse systemd version from 'systemctl --version'") |
There was a problem hiding this comment.
If this warning were moved to inside get_systemd_version, it could include the error and output that failed to be parsed.
There was a problem hiding this comment.
Yikes I'm struggling...
systemd.py doesn't have an ability for me to reference self.log, and I figured I'd emit self.log.critical at one point there if failing to call systemctl at all - not just failing to parse the response. What do you think makes sense to do?
def get_systemd_version():
"""
Returns systemd's major version, or None if failing to do so.
"""
try:
version_response = subprocess.check_output(["systemctl", "--version"])
except subprocess.CalledProcessError:
warnings.warn("Failed to call 'systemctl --version', is systemd installed?")
raise
try:
# Example response to parse from Ubuntu 22.04:
#
# systemd 249 (249.11-0ubuntu3.9)
# +PAM +AUDIT +SELINUX +APPARMOR +IMA +SMACK +SECCOMP +GCRYPT +GNUTLS +OPENSSL +ACL +BLKID +CURL +ELFUTILS +FIDO2 +IDN2 -IDN +IPTC +KMOD +LIBCRYPTSETUP +LIBFDISK +PCRE2 -PWQUALITY -P11KIT -QRENCODE +BZIP2 +LZ4 +XZ +ZLIB +ZSTD -XKBCOMMON +UTMP +SYSVINIT default-hierarchy=unified
#
version = int(float(version_response.split()[1]))
return version
except:
warnings.warn(f"Failed to parse systemd version from 'systemctl --version' response: {version_response}")
return NoneThere was a problem hiding this comment.
@minrk can push a commit to this PR adjusting it as you think makes sense?
There was a problem hiding this comment.
Pushed basically exactly what you had there.
| elif systemd_version < SYSTEMD_REQUIRED_VERSION: | ||
| self.log.critical( | ||
| f"systemd version {SYSTEMD_REQUIRED_VERSION} or higher is required, version {systemd_version} is used" | ||
| ) | ||
| sys.exit(1) | ||
| elif systemd_version < SYSTEMD_LOWEST_RECOMMENDED_VERSION: | ||
| self.log.warning( | ||
| warnings.warn( |
There was a problem hiding this comment.
warnings almost always want a stacklevel (usually but not always 2), but I think this is actually a case where it wouldn't be helpful, so 👍 here.
No description provided.