-
Notifications
You must be signed in to change notification settings - Fork 70
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
cli: Handle missing commands gracefully #785
cli: Handle missing commands gracefully #785
Conversation
Thank you for contributing to the Leapp project!Please note that every PR needs to comply with the Leapp Guidelines and must pass all tests in order to be mergable.
To launch regression testing public members of oamg organization can leave the following comment:
Please open ticket in case you experience technical problem with the CI. (RH internal only) Note: In case there are problems with tests not being triggered automatically on new PR/commit or pending for a long time, please consider rerunning the CI by commenting leapp-ci build (might require several comments). If the problem persists, contact leapp-infra. |
f0faa45
to
d72b339
Compare
/rerun |
Copr build succeeded: https://copr.fedorainfracloud.org/coprs/build/4685849 |
Testing Farm request for RHEL-8.6-rhui/4656862;4685849 regression testing has been created. |
Testing Farm request for RHEL-7.9-rhui/4656862;4685849 regression testing has been created. |
Testing Farm request for RHEL-8.6.0-Nightly/4656862;4685849 regression testing has been created. |
Testing Farm request for RHEL-7.9-ZStream/4656862;4685849 regression testing has been created. |
The CLI for the leapp utility is provided nowadays by leapp-repository packages. However, some people are used to install just `leapp` instead of specific repository as we suggest (e.g. `leapp-upgrade`). To handle the situation we decided to provide a proper error msg to users suggesting what they should do. Inspired by DNF (hi guys \o) we decided to set virtual provides in leapp-repository packages: leapp-command(CMD) per each leapp CMD provided by particular package, so users can be suggested to install missing packages e.g. by: dnf install 'leapp-command(upgrade)' when they want to run `leapp upgrade`. Same for all other commands. See the following PR in leapp handling this problem: oamg/leapp#785
/rerun |
Copr build succeeded: https://copr.fedorainfracloud.org/coprs/build/4746223 |
Testing Farm request for RHEL-8.6-rhui/4744089;4746223 regression testing has been created. |
Testing Farm request for RHEL-7.9-rhui/4744089;4746223 regression testing has been created. |
Testing Farm request for RHEL-8.7.0-Nightly/4744089;4746223 regression testing has been created. |
Testing Farm request for RHEL-8.6.0-Nightly/4744089;4746223 regression testing has been created. |
The CLI for the leapp utility is provided nowadays by leapp-repository packages. However, some people are used to install just `leapp` instead of specific repository as we suggest (e.g. `leapp-upgrade`). To handle the situation we decided to provide a proper error msg to users suggesting what they should do. Inspired by DNF (hi guys \o) we decided to set virtual provides in leapp-repository packages: leapp-command(CMD) per each leapp CMD provided by particular package, so users can be suggested to install missing packages e.g. by: dnf install 'leapp-command(upgrade)' when they want to run `leapp upgrade`. Same for all other commands. See the following PR in leapp handling this problem: oamg/leapp#785
Testing Farm request for RHEL-7.9-ZStream/4744089;4746223 regression testing has been created. |
Testing Farm request for RHEL-7.9-ZStream/4744089;4746223 regression testing has been created. |
/rerun |
Copr build succeeded: https://copr.fedorainfracloud.org/coprs/build/4746686 |
be46ad8
to
a5b25ce
Compare
/packit copr-build |
@pirat89 Sorry, being lazy here - how about we point the user to search for any
If they still invoke leapp with bad argument, the second invocation will be pretty informative
|
Testing Farm request for RHEL-7.9-rhui/4746605;4746686 regression testing has been created. |
Testing Farm request for RHEL-8.7.0-Nightly/4746605;4746686 regression testing has been created. |
Testing Farm request for RHEL-8.6.0-Nightly/4746605;4746686 regression testing has been created. |
Testing Farm request for RHEL-7.9-ZStream/4746605;4746686 regression testing has been created. |
Testing Farm request for RHEL-7.9-ZStream/4746605;4746686 regression testing has been created. |
@fernflower this could be confusing for users. They will not find the
When we can easily print the command, we should put it there instead of the asterisk. As on RHEL 7 we know that leapp-upgrade packages will be always installed with leapp, we are ok to refer to DNF as we know it will be installed as well - even when in such a case, seeing this msg will most likely mean a typo in the command :) |
@pirat89 Well, they can run @Rezney @vinzenz wdyt? Let's agree on the wording and be done with this patch. |
@fernflower let's discuss that. in both cases, the command is wrong as apostrophes/quotes are missing. Additionally, asterisk is not good as it will start to try to install all packages providing any leapp command. The idea is to install the right package providing the desired command. So no, I am taking the replacement of the asterisk by the particular command as good practice. |
Previously when leapp was executed but no cli commands were installed, the leapp execution failed with an exception. This patch will add specific handling to this particular scenario and return a 1 exit code. Co-authored-by: Petr Stodůlka <pstodulk@redhat.com> Co-authored-by: Inessa Vasilevskaya <ivasilev@redhat.com> Signed-off-by: Vinzenz Feenstra <vfeenstr@redhat.com>
20adca4
to
87ba733
Compare
@vinzenz @fernflower so I have 1 good and 2 bad news. The fix works as expected for the commands when there is not any command installed:
But it has different behaviour when any command is provided:
And when no commands are provided, also CLI options are treated as commands:
@vinzenz @fernflower @oamg/developers how we will deal with this? To be honest, I am not strongly convinced that this is blocker now. As in case some commands are available, right now we know we are basically still ok. And if nothing is available, well, we expect that people will always call at least one command, so it should be still ok, like:
If we can fix it quickly, let's fix it. If not, I think we should go with this. But I want to know your opinion on that as well. |
ef59117
to
8503174
Compare
/rerun |
Copr build succeeded: https://copr.fedorainfracloud.org/coprs/build/4755951 |
Testing Farm request for RHEL-7.9-rhui/4755849;4755951 regression testing has been created. |
@pirat89 Only a straightforward easy fix comes to my mind, please see the latest commit (we can definitely tune the error message wording, it is just an example).
Imho we have solved the original problem with this patch - there is no more ugly confusing traceback and the user actually gets pointed in the right direction. |
Testing Farm request for RHEL-8.7.0-Nightly/4755849;4755951 regression testing has been created. |
Testing Farm request for RHEL-8.6.0-Nightly/4755849;4755951 regression testing has been created. |
Testing Farm request for RHEL-7.9-ZStream/4755849;4755951 regression testing has been created. |
Testing Farm request for RHEL-7.9-ZStream/4755849;4755951 regression testing has been created. |
Agree. I think this is good enough. Works as written. Thanks :) Merging |
## Packaging - bumped leapp-framework to 3.1 (oamg#677) ## Framework ### Fixes - Fixed a problem where passing environment variables to an executed child process modified the environment variables of the parent process (oamg#784) - Ignore invalid FQDNs (oamg#790) ### Enhancements - Deprecate `reporting.(Tags|Flags)`, replaced by `reporting.Groups` (oamg#677, oamg#781, oamg#788) - Introduce `is_inhibitor` function (oamg#677) - Introduce a `Blob` model field (oamg#789) - Introduce new report JSON schema v1.2.0 (default: 1.1.0) (oamg#677) ## Leapp (tool) ### Fixes - Handle missing CLI commands gracefully (oamg#785) - Requires to be executed by root only (oamg#775)
## Packaging - bumped leapp-framework to 3.1 (oamg#677) ## Framework ### Fixes - Fixed a problem where passing environment variables to an executed child process modified the environment variables of the parent process (oamg#784) - Ignore invalid FQDNs (oamg#790) ### Enhancements - Deprecate `reporting.(Tags|Flags)`, replaced by `reporting.Groups` (oamg#677, oamg#781, oamg#788) - Introduce `is_inhibitor` function (oamg#677) - Introduce a `Blob` model field (oamg#789) - Introduce new report JSON schema v1.2.0 (default: 1.1.0) (oamg#677) ## Leapp (tool) ### Fixes - Handle missing CLI commands gracefully (oamg#785) - Requires to be executed by root only (oamg#775) Signed-off-by: Petr Stodulka <pstodulk@redhat.com>
## Packaging - bumped leapp-framework to 3.1 (#677) ## Framework ### Fixes - Fixed a problem where passing environment variables to an executed child process modified the environment variables of the parent process (#784) - Ignore invalid FQDNs (#790) ### Enhancements - Deprecate `reporting.(Tags|Flags)`, replaced by `reporting.Groups` (#677, #781, #788) - Introduce `is_inhibitor` function (#677) - Introduce a `Blob` model field (#789) - Introduce new report JSON schema v1.2.0 (default: 1.1.0) (#677) ## Leapp (tool) ### Fixes - Handle missing CLI commands gracefully (#785) - Requires to be executed by root only (#775) Signed-off-by: Petr Stodulka <pstodulk@redhat.com> Signed-off-by: Petr Stodulka <pstodulk@redhat.com>
Previously when leapp was executed but no cli commands were installed,
the leapp execution failed with an exception.
This patch will add specific handling to this particular scenario and
return a 1 exit code.
Signed-off-by: Vinzenz Feenstra vfeenstr@redhat.com
Jira ref.: OAMG-7148