| kparal | |
| tflink | |
| mkrizek | |
| jskladan |
Package installation process depends on profile. For development profile, it is
checked whether the packages are installed. If not, exception is raised and user
is prompted to install them manually. Production profile tries to install all
specified packages (without sudo).
Task initiator now pops only machine part of environment and the rpm list is
passed to task executor, packages are installed by the task executor itself.
run taskotron with simple formula (with both installed and not installed rpms) in
development and production mode
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
If we start relying on dnf, there should be a dependency in the spec file.
As for sudo, maybe it's now time to decide what do we want to do with user privileges. Should we assume we run under root? Should it be configurable? Should we run it through sudo always, just in case (that doesn't preclude running under root, it seems to work just fine)? @tflink, thoughts?
Also, what if the developer runs it in dev mode, but wants the installation to happen automatically (i.e. it is running in disposable clients mode), will we add a config/cli option for it?
What about this:
| libtaskotron/runner.py | ||
|---|---|---|
| 165–167 | This method name implies it should do validation of content in formula (similar to _validate_input()). Instead, you perform actions The method should be named differently (and maybe we can put some parts of that into rpm_utils instead of runner, I think that would be nice). | |
| 176 | I can imagine a situation where you would like to install all the rpms in a single transaction instead of one by one. Let's say you want to install foo1 which either requires bar1 or bar2, so that's why you put foo1 and bar1 into the formula. Can we do the installation in a single transaction? This will also make sure that any deps error that we encounter we find immediately, rather then some time after many other packages were installed. | |
| 177–180 | In this case it might be better to use stderr=subprocess.STDOUT and then print the output in full, instead of having the lines separated by stdout and stderr? Otherwise we might have troubles distinguishing where exactly that error line was, i.e. putting it back together. | |
| 183–184 | This message makes sense for dnf list installed, but not that much for dnf install (it would make sense to phrase it something along the lines of "Installation failed for ..."). You'll probably need to split it between those two cases. | |
| 217–218 | This will crash if environment key is not present, right? | |
I created a table from discussion with @kparal, showing whether to install or raise under different circumstances. We need a better way how to distinguish whether we are running as task initiator or executor. Flag --local passed to runtask would do it and it could also be used in developer profile to install packages and run destructive tasks.
| production | develop | |
| bare metal | install | raise (install with --local) |
| remote | install (run with --local) | install |
In other words, Lukas needs to distinguish different run modes, which are not yet distinguishable. Most importantly, distinction between running a task in development profile locally, and on a remote machine (using --ssh or similar). Both look the same formula-wise. We figured that my --local cmdline option from T408 could be perfect for this. Not only it would confirm destructive task execution, but it would help us distinguish between task initiator and task client. The initiator would always add --local when running runtask over ssh, and the client would easily know that "I'm the machine this task actually runs on". After a bit of thinking, it seems to me that merging these two use cases into a single option makes sense, with the meaning "run this task here, no questions asked". The end result is that when developer forces a destructive task to be run on his local machine with --local, it also implies that RPM requirements are installed automatically, which I think fits in the concept of a "destructive" (system changing) task.
I already asked @lbrabec to implement --local in a separate patch and make this patch depend on that one. If there are any questions or concerns, please tell us, thanks.
| libtaskotron/rpm_utils.py | ||
|---|---|---|
| 105–107 | Just to make sure it is clear we install packages from repository by name, and not from a file on disk, let's clarify: Install packages from system repositories using DNF. You need to have enough permissions to run ``dnf`` through ``sudo``. :param pkgs: package names to be installed, e.g. ``['pidgin']``, or any other specifier support by DNF install command :type pkgs: list of str | |
| 110–111 | I'd rather if you used a native list to give to Popen, instead of joining the command line using join() and the splitting it on the next line. If someone gives you an argument containing a space (like "@Print Support"), this will break. So: cmd = ['sudo', 'dnf', 'install', '-y'].extend(pkgs) | |
| 124 | Please clarify whether this should be just name, or NEVR, or NEVRA, or if all of that works... and if not, can we make it work? | |
| 131 | I've had just a quick look, but it seems that you can run either .count() or .result on that query, which gives you what you want in a somewhat cleaner fashion. | |
| libtaskotron/runner.py | ||
| 180–181 | I'm not sure how difficult this is, but if you could print this in a properly shell-escaped syntax, that would be cool. | |
A few nitpicks in the review, but in general I think the code looks OK now. We will probably want some unit tests, I guess at least for the prepare_task() method to make sure we do the right thing in different use cases (regarding the decision table provided). But that will require --local patches having been completed first. Do you want to wait for that, or rather push this asap and adjust the code later?
| libtaskotron/rpm_utils.py | ||
|---|---|---|
| 104 | To give you even more options besides pkgs and items, you could also use spec or specs, that's what dnf uses in its manpage :) Pick what you think sounds best and most straightforward. (I'd personally probably stayed with pkgs, even though it's not completely precise). | |
| 109 | Of coursed I wanted to type "supported" :-) And ``dnf install`` probably, to make it look like a command. | |
| 129 | Does "items to be checked whether they are installed" sound clearer? I know it's a bit clunky. But currently it sounds like I would want to install them. | |
| 130 | same remark as above | |
| 137 | I guess this should not be here. But it is a good idea to log this under debug, both here and in install() method. In install(), I'd even go that far to log it under info, because it might take considerable time. Something like: log.info("Running command: %s", cmd)Using debug level in is_installed() is fine, it will not take very long. | |
| libtaskotron/runner.py | ||
| 169 | Please add a docstring. | |
Adding Tim and Josef to reviewers, in case they want to look at it, this is a core change.
One more thing, you still need to add DNF to the spec file, now that we depend on it.
| libtaskotron.spec | ||
|---|---|---|
| 16–17 | Maybe you should add a comment, that makes it clear :) | |
Just recently I sorted it by alphabet, to make comparison with requirements.txt easier... ;)