don't uninstall cli subcommand when there are remaining apps #1046
Conversation
|
run linux integration tests |
|
run mac integration tests |
| @@ -510,11 +510,8 @@ def _uninstall(package_name, remove_all, app_id, cli, app, skip_confirmation): | |||
| return 0 | |||
|
|
|||
| package_manager = get_package_manager() | |||
| err = package.uninstall( | |||
| package.uninstall( | |||
There was a problem hiding this comment.
Why remove this error checking?
There was a problem hiding this comment.
there is no error getting returned here, but an exception can be raised and would be caught by the main function.
| @@ -24,28 +24,29 @@ def uninstall(pkg, package_name, remove_all, app_id, cli, app): | |||
| :rtype: None | |||
There was a problem hiding this comment.
The docstring is wrong and doesn't include cli and app arguments, but does include a (nonexistent) init_client argument. Can you please update this to include the correct information.
dcos/package.py
Outdated
| if subcommand.uninstall(package_name): | ||
| uninstalled = True | ||
| if cli is False and app is False: | ||
| app = True |
There was a problem hiding this comment.
It's not clear to me what these lines of code are intended to do. Why do we check if app is false, and if so, set it immediately to true? I would think that passing in app or cli as False would mean that we explicitly didn't want to uninstall one or the other of these. We either need a comment or some better naming of things to make what is going on here more apparent.
There was a problem hiding this comment.
basically those variables are tied to command line options, having both to false means the user didn't pass --cli nor --app, in that situation we are trying to uninstall both if possible, otherwise fallback to app uninstall only.
There was a problem hiding this comment.
We should definitely have a comment about this here then, because that is not at all obvious.
dcos/package.py
Outdated
|
|
||
| # remove CLI only if there would be no remaining apps after uninstall | ||
| cli = (remove_all or not app_id | ||
| or installed_app.count(app_id) == len(installed_app)) | ||
|
|
||
| if app and installed_app: | ||
| if pkg.uninstall_app(package_name, remove_all, app_id): | ||
| uninstalled = True |
There was a problem hiding this comment.
I think we should have separate variables for app_ininstalled and cli_uninstalled so we can print a more useful error message about what was / wasn't uninstalled if there was an error.
Also, I don't know if this is possible, but we should print a warning if the user requested to remove the CLI, but we didn't because there is an app still installed that relies on it.
There was a problem hiding this comment.
if the user requests to remove the CLI it gets removed, the "smart" logic only applies when there no --cli or --app flags.
There was a problem hiding this comment.
Also, to clarify, using --cli and --app at the same time is unsupported (it will trigger an invalid usage error, they are defined as mutually exclusive options).
There was a problem hiding this comment.
That in itself seems odd. So passing --app alone will only uninstall the app. Passing --cli alone will only uninstall the CLI. And passing neither will uninstall both.
...
But passing both installs neither?
I would think that passing both should also uninstall both.
There was a problem hiding this comment.
I believe this is because they originally meant "cli only" or "app only", as such it didn't make sense to use both at the same time. But we could change it along with this new behaviour, are you suggesting the following?
package uninstall hello > uninstall app(s), uninstall CLI unless apps remain after uninstall
package uninstall hello --cli > uninstall CLI
package uninstall hello --app > uninstall app(s)
package uninstall
8000
hello --app --cli > uninstall app(s) and CLI
There was a problem hiding this comment.
I've moved the double false check at the command level, and use the None value for the cli param, cf. last commit.
|
Yes. Exactly this. It would be good if the documentation mentioned these
semantics as well (not sure how detailed they are at the moment).
|
b2bafd8 to
02444d2
Compare
|
run all integration tests |
dcos/package.py
Outdated
| # delete the CLI only when no apps would remain. | ||
| if cli is None: | ||
| cli = (remove_all or not app_id | ||
| or installed_app.count(app_id) == len(installed_app)) |
There was a problem hiding this comment.
I find this logic hard to follow (see comment below for a suggested alternative).
cli/dcoscli/package/main.py
Outdated
| if app is False and cli is False: | ||
| cli = None | ||
| app = True | ||
|
|
There was a problem hiding this comment.
I don't think we should do this logic out here, I would rather push this down into the actual package.uninstall() command.
dcos/package.py
Outdated
| if cli and installed_cli: | ||
| if not subcommand.uninstall(package_name): | ||
| raise DCOSException("Couldn't uninstall subcommand") | ||
|
|
There was a problem hiding this comment.
I don't really like the way the logic flows in this function. It's really hard to follow. Specifically, it's hard to understand the semantics around the values of cli and app (especially regarding the 3rd value of None that cli can now take on). It makes more sense to me to change the entire logic of this function to the following (where the comments match the code exactly):
# Passing `app == True` means that the user supplied an explicit `--app` flag on
# the command line. This forces an unconditional uninstall of the app associated
# with the supplied package (with different semantics depending on the values
# of `remove_all` and `app_id` as described in the docstring for this function).
if app and installed_app:
if not pkg.uninstall_app(package_name, remove_all, app_id):
raise DCOSException("Couldn't uninstall package")
# Passing `cli == True` means that the user supplied an explicit `--cli` flag on
# the command line. This forces an unconditional uninstall of the CLI associated
# with the supplied package.
if cli and installed_cli:
if not subcommand.uninstall(package_name):
raise DCOSException("Couldn't uninstall subcommand")
# Passing both `app == False` and `cli == False` means that the user didn't
# supply either `--app` or `--cli` on the command line. In this situation we
# uninstall the app associated with the supplied package (if it exists) as
# if the user had explicitly passed `--app` on the command line. However,
# we only uninstall the CLI associated with the package if the app being
# uninstalled is the last one remaining on the system. Otherwise, we leave the
# CLI in place so other instances of the app can continue to interact with it.
if installed_app:
if not pkg.uninstall_app(package_name, remove_all, app_id):
raise DCOSException("Couldn't uninstall package")
(Is this logic even correct? I don't understand why
`not app_id` is sufficient to imply all instances removed)
all_instances_removed = remove_all or \
not app_id or \
installed_app.count(app_id) == len(installed_app)
if installed_cli and all_instances_removed:
if not subcommand.uninstall(package_name):
raise DCOSException("Couldn't uninstall subcommand")
In general, overloading a boolean value to treat None as a 3rd option is very non-intuitive and should probably be avoided at all costs. Being explicit instead of clever can go a long way.
There was a problem hiding this comment.
Is this logic even correct? I don't understand why
not app_idis sufficient to imply all instances removed
Without the --all flag, not passing an app_id assumes there is a single app to uninstall, otherwise fails with multiple apps installed, please set --all or --app-id. So that condition seems correct to me. However, it can probably be simplified as : remove_all or len(installed_app) <= 1 (mean
9E19
ing this is either a CLI only app, or a single app existed and was removed)
cli/dcoscli/data/help/package.txt
Outdated
| @@ -21,7 +21,7 @@ Usage: | |||
| dcos package repo remove <repo-names>... | |||
| dcos package search [<query> --json] | |||
| dcos package uninstall <package-name> | |||
| [--cli | [--app --app-id=<app-id> --all --yes]] | |||
| [--cli --app --app-id=<app-id> --all --yes] | |||
There was a problem hiding this comment.
I don't know how to do it off the top of my head, but it would be great if we could force docopt to recognize that --app-id and --all are only valid if you also pass --app.
There was a problem hiding this comment.
This is not the case, for example dcos package uninstall hello-world --all is valid.
Actually, I'm not convinced about our change here (to allow both --cli and --app) :
- in terms of semantics, those options are described as
CLI onlyorapplication only(https://github.com/bamarni/dcos-cli/blob/02444d2cc9d7619f22eecadf7ef710aa67977d2b/cli/dcoscli/data/help/package.txt#L48-L53), so it's kind of strange to use both. - it doesn't align with
dcos package installcommand, which would still have them as mutually exclusive (https://github.com/bamarni/dcos-cli/blob/02444d2cc9d7619f22eecadf7ef710aa67977d2b/cli/dcoscli/data/help/package.txt#L12).
There was a problem hiding this comment.
Or I think you meant : --app-id and --all are not valid if you pass --cli?
There was a problem hiding this comment.
Yeah, OK, that makes sense. I'm OK going back to the old semantics if the docs (and other functions) are explicit about the ONLY part (meaning put the old docopt string back in place). That shouldn't change anything about the new code flow I suggested though (other than to mention in the comments that only one of cli or app will ever be true at a given time).
56a98c8 to
0693a22
Compare
|
run all integration tests |
No error is returned here, instead an exception is raised and caught by the main function.
). Prior to this change, removing any app which has a subcommand would automatically remove the subcommand too. This refactors the logic to remove the subcommand only when no apps are expected to remain after uninstall. https://jira.mesosphere.com/browse/DCOS-17880
|
rerun linux integration tests |
|
rerun mac integration tests |
|
rerun windows integration tests |
). Prior to this change, removing any app which has a subcommand would automatically remove the subcommand too. This refactors the logic to remove the subcommand only when no apps are expected to remain after uninstall. https://jira.mesosphere.com/browse/DCOS-17880
|
rerun all integration tests |
). Prior to this change, removing any app which has a subcommand would automatically remove the subcommand too. This refactors the logic to remove the subcommand only when no apps are expected to remain after uninstall. https://jira.mesosphere.com/browse/DCOS-17880
). Prior to this change, removing any app which has a subcommand would automatically remove the subcommand too. This refactors the logic to remove the subcommand only when no apps are expected to remain after uninstall. https://jira.mesosphere.com/browse/DCOS-17880
|
run all integration tests |
No error is returned here, instead an exception is raised and caught by the main function.
Prior to this change, removing any app which has a subcommand would automatically remove the subcommand too. This refactors the logic to remove the subcommand only when no apps are expected to remain after uninstall. https://jira.mesosphere.com/browse/DCOS-17880
When there are multiple apps from a given package, which also happens to have a subcommand, uninstalling any of these app would remove the subcommand :
This PR changes the uninstall behaviour so that no subcommand is removed as long as there is at least one remaining app after uninstalling.
I'm not able to add tests for this at the moment as our integration tests use a
helloworldpackage from a universe server which doesn't support the--optionsflag, I'm currently investigating this with @larioj and extracted these tests refactoring into #1051.https://jira.mesosphere.com/browse/DCOS-17880