Salt: modules: *pkg: upgrade:The current api of function 'upgrade' is dangerous

[stardust85]

salt '*' pkg.upgrade openssl

is the same as

salt '*' pkg.upgrade refresh=openssl # doesn't make sense

and causes that all systems are COMPLETELY upgraded (ALL PACKAGES, not only openssl), because salt.utils.is_true('openssl') returns True, not an exception. This happened to my colleague and caused serious troubles to the my whole team.

The API of the function 'upgrade' should be changed in all modules exporting virtual module pkg so that argument name is on the first place the same way as it is in install.

yumpkg (see pull request #26022) and aptpkg (see issue #16293) already support upgrade of specific packages instead of whole system. In yumpkg by passing parameters 'name' or 'packages', in aptpkg by --only-upgrade in install.

The principle of the least surprise should be followed here. I understand the consequences of this change, but the worst-case scenario is that after upgrade to new SaltStack someone will run.

salt '*' pkg.upgrade False # not so common I think
and will get 0 upgrades because package False doesn't exist. I think this is less evil than what happened to my team and can happen to anyone.

[jfindlay]

@stardust85, thanks for your work on this.

[stardust85]

Also discussed in https://github.com/saltstack/salt/issues/7179

Two guys were hurt by this again. Two years after reporting people still accidentally upgrade whole system instead of one package.

This is another argument for (and I hate to say that, as a salt user and contributor) leaving Salt.

[stardust85]

Ok, to be more constructive, I propose this fix:

1. deprecate old behavior so it prints warnings, update docs
2. one release later, remove the dangerous functionality

[damon-atkins]

Simple fix is your need to pass a magic package name for all packages to be upgraded.
e.g. ++all++ e.g.

salt \* pkg.upgrade ++all++
salt \* pkg.upgrade name=++all++
salt \* pkg.upgrade  # gives an error

Anyone from the salt team think this is a bad idea? Please add your github reaction to this comment.
Anyone know of a package called ++all++ that would cause an issue if this was the magic name.

[stardust85]

@damon-atkins This is a funny solution, but it should work :)
I think it it's time to deprecate pkg.upgrade without args in favor of this syntax

[stardust85]

@basepi any input on this?

[racooper]

This is a duplicate of #7179 which has been discussing this issue for several years now - 2013 I believe. The suggestion over there that to me seems best is to leave pkg.upgrade to upgrade the whole system, and pkg.update to update an individual package. This apparently mimics the existing behavior of the ebuild module. if pkg.upgrade were run with any arguments, it would produce an error and abort in that situation.

[damon-atkins]

The idea is to avoid an accident.
Does not matter if it is pkg.doit it still needs to be pkg.doit ++all++

[stardust85]

Ok, not to have this in 2 issues, I'm closing this as a duplicate of #7179. When I discussed in #7179 somebody asked me to open a new issue if I remember well.