procServUtils autocomplete and new commands #34
Conversation
Reset-failed when removing procs
|
@mdavidsaver and @mark0n, do you have opinions on this? Looks good to me. |
|
My first benchmark for dependencies is presence in the Debian package repository. All three have been present since at least Debian 8 (10 is stable atm). argcomplete tabulate termcolor argcomplete and tabulate appear at a glance to be healthy projects with recent releases and links to VCS repos. termcolor appears to be defunct, with no release since 2011, and no authoritative VCS repository listed. |
|
Personally, I would make this three optional dependencies with some |
procServUtils/README.md
Outdated
| ``` | ||
| Activate global arcomplete completion: | ||
| ```bash | ||
| sudo activate-global-python-argcomplete |
There was a problem hiding this comment.
This step should be named as optional, maybe referring to https://pypi.org/project/argcomplete/#activating-global-completion
fyi. It tries to write /etc/bash_completion.d/python-argcomplete.sh by default.
There was a problem hiding this comment.
Ok, I will list it as an optional step!
procServUtils/manage.py
Outdated
| @@ -207,11 +222,18 @@ def delproc(conf, args): | |||
| stopproc(conf, args) | |||
|
|
|||
| _log.info("Disabling service procserv-%s.service", args.name) | |||
| SP.check_call([systemctl, | |||
| SP.call([systemctl, | |||
There was a problem hiding this comment.
Why change to the un-checked call? Have you seen this step fail? As a general rule I don't like the idea of ignoring failures.
There was a problem hiding this comment.
I remember seeing this fail, but I cannot reproduce the problem now (it was months ago), so for the moment I will revert back to the check_call method.
procServUtils/manage.py
Outdated
| _log.info('Trigger systemd reload') | ||
| SP.check_call([systemctl, | ||
| argusersys, | ||
| 'daemon-reload'], shell=False) |
There was a problem hiding this comment.
I'm not sure about the ordering here, but I think the daemon-reload should come first. As I understand it, a reload is needed after manually editing any systemd config files, eg. before any new units are recognised.
I'm also wondering it an explicit disable is needed for the original service name? Have you verified that the symlinks are being cleaned up? (eg. ls -l /etc/systemd/system/multi-user.target.wants/)
There was a problem hiding this comment.
I agree with @mdavidsaver, the order should be:
- stop and disable old unit (calling disable is required if we don't want to leave any mess behind)
- modify unit files
- daemon-reload
- enable and start new unit
There was a problem hiding this comment.
Ok, good catch! I copied this logic from the addproc function, so I will fix the order also on that function.
| @@ -29,4 +29,5 @@ def run(self): | |||
| ('lib/systemd/user-generators', ['systemd-procserv-generator-user']), | |||
| ], | |||
| cmdclass = { 'install_data': custom_install_data }, | |||
| #install_requires=['argcomplete', 'tabulate', 'termcolor'], | |||
There was a problem hiding this comment.
If these are to be unconditional dependencies, then install_requires needs to be uncommentated.
There was a problem hiding this comment.
The problem here is that the project uses distutils to install the python scripts, which does not support the install_requires directive. Maybe it's possible to use pip, but I'm not sure how to integrate it with the makefiles.
For the moment I will follow your suggestion to make this packages optional using the try: ... import mechanism and defining some fallback behaviors.
There was a problem hiding this comment.
Ya... I was a late and reluctant convert to using setuptools. I had hoped that distutils would be cleaned up to include the useful parts. However, the opposite has happened. With so much external tooling with invasive hooks into distutils, it doesn't seem like it can reasonable be changed. At least setuptools availability is ubiquitous now.
Although I don't think this module will ever be usable in eg. a virtualenv, switching to setuptools is probably reasonable. Doing this may necessitate changes to the custom_install_data though.
|
Hello @mdavidsaver, |
|
I made the three packages optional, adding some fallback functions that emulate almost the same behavior as before this PR. I redirected the standard error of the I didn't remove the |
|
Lastly, since the |
Hello,
this PR adds some functionalities to the procServUtils scripts:
argcompletestatusas a table with color support, to be easier to readTo enable these functionalities three python dependencies are needed:
argcomplete,tabulateandtermcolor. Unfortunately the current installation process does not support installing dependencies so one has to install them manually through pip.Do you think the installation process can be modified to use pip to automatically handle dependencies?
All the changes here are compatible with existing configuration files and do not break compatibility (hopefully).