Fixes #28097 - Allow packages update with no packages #286

mbacovsky · 2019-10-21T12:52:29Z

This patch lets packages update command to be invoked
without any args to update all packages.

This patch lets packages update command to be invoked without any args to update all packages.

theforeman-bot · 2019-10-21T12:52:37Z

ntkathole · 2019-10-21T14:25:44Z

Aren't we allowing user here to perform upgrade with foreman-maintain packages update command without actually running foreman-maintain upgrade run command ? Currently I am not sure how will this have side effects, but, can we consider showing warning to use foreman-maintain upgrade run command rather than allowing it without args?

mbacovsky · 2019-10-21T21:25:11Z

@ntkathole it is already possible we are making it just a bit more easy to use. I see your concern though, updating everything is probably too risky.

What others do think? Should we try to detect attempt to update everything and prevent it with deferring to f-m upgrade? @kgaikwad, @upadhyeammit, @gnurag

upadhyeammit · 2019-10-22T12:01:36Z

@ntkathole it is already possible we are making it just a bit more easy to use. I see your concern though, updating everything is probably too risky.

What others do think? Should we try to detect attempt to update everything and prevent it with deferring to f-m upgrade? @kgaikwad, @upadhyeammit, @gnurag

I think we should put warning but should not prevent it, because we are doing this change to make it easier to update all packages[if required]. I can think of below use cases, if I missed anything then request to add,

Update to packages which are of beta, or next major release.
If user is doing this then he knows what are the implications. Because nowhere in document we have mentioned to manually change repositories to either beta or major release and do upgrade on GA.
Update to EPEL packages, or upstream repository packages.
In this use case I tried the after and before in confine block and it did not prioritize run. So here also user should be knowing what actions he is doing.
Update to next z stream.
Same as 1. but with less impact, we may or may not need to run the installer again.

Considering all these I feel warning should be sufficient.

kgaikwad · 2019-10-22T17:22:22Z

@mbacovsky,
As it is a risky operation and considering users don't always read the docs before running any command, I agree with @ntkathole & @upadhyeammit on showing a warning message than updating everything.
Though foreman-maintain packages update command is running a installer command after packages update, there might be a chance that manual step(s) will be involved during upgrade from one version to other. That's why it is better not to leave system in broken state.

mbacovsky · 2019-10-30T11:48:03Z

Thanks everybody for your input. I'm going to print a warning that packages update should not be used instead of upgrade run.

@kgaikwad does your suggestion mean we shouldn't allow packages update with no arg?

kgaikwad · 2019-10-30T12:26:37Z

Thanks everybody for your input. I'm going to print a warning that packages update should not be used instead of upgrade run.

@kgaikwad does your suggestion mean we shouldn't allow packages update with no arg?

Yes, by considering the points(1st & 3rd) mentioned by @upadhyeammit and IIRC, in the past few issues were reported by users around yum update command.
If we would like to implement it in this way then it would be good to have more suggestions on this change.

mbacovsky · 2019-10-30T13:31:37Z

I've added the confirmation.

jameerpathan111 · 2019-10-31T11:29:43Z

@mbacovsky
The warning message is not shown properly when I ran foreman-maintain packages update --assumeyes command.

================================================================================
Confirm update all is intentionall: 

WARNING: No speciffic packages to update were provided so we are going to update all available packages.
It is recommended to update everything only as part of upgrade of the Satellite to the next version. 
To Upgrade to next version use 'foreman-maintain upgrade'.

                                                                      [OK]      ns? (assuming yes)
--------------------------------------------------------------------------------
Confirm installer run is allowed:

Line Do you want to proceed with update of everything regardless of the recommendations?, [y(yes), q(quit)] y is missing.

mbacovsky · 2019-11-05T12:02:38Z

@jameerpathan111 the output should be fixed now. I've seen the same behavior elsewhere too so it should be fixed globally.

@kgaikwad, I agree that allowing users to update everything is not the ideal but it still seems better then forcing users to come up with workaround by providing wildcards or turning the locking off completely. So I used the wording indicating that updating everything is risky and what we recommend but allowed to proceed. Is that acceptable for you? Would it make sense to disable --assumeyes for this situation?

jameerpathan111 · 2019-11-06T08:45:33Z

@jameerpathan111 the output should be fixed now. I've seen the same behavior elsewhere too so it should be fixed globally.

Yes, it seems to be fixed now.

kgaikwad · 2019-11-06T19:30:03Z

Is that acceptable for you? Would it make sense to disable --assumeyes for this situation?

+1 for the warning message and asking user before proceeding further.
As it is the risky and probably won't be possible to bring back to original state once ran so I think we should avoid having --assumeyes option for this at least in the beginning.

jameerpathan111 · 2019-11-11T13:38:20Z

@mbacovsky @kgaikwad if we remove --assumeyes option it'll cause problem in customer automation.

mbacovsky · 2019-11-11T13:59:07Z

@jameerpathan111 I guess that is the point. This shouldn't be automated at all. Currently we do not support update without parameters. The --assumeyes should stay with the command we will just ignore --assumeyes on parameterless update. We can inform that the option was ignored on purpose. Would that be acceptable?

jameerpathan111 · 2019-11-12T04:59:01Z

@jameerpathan111 I guess that is the point. This shouldn't be automated at all. Currently we do not support update without parameters. The --assumeyes should stay with the command we will just ignore --assumeyes on parameterless update. We can inform that the option was ignored on purpose. Would that be acceptable?

yes

mbacovsky · 2019-11-14T10:23:16Z

Updated according to our previous discussion.

Also please note I loosend the HashSyntax cop a bit from Hash_rocket to no_mixed_keys. The reason was it fails on keyword argument and it seems there is no remedy for that. I checked the setting on Foreman core and this cop is turned off there. If anybody has concerns with this change please speak up.

gnurag · 2019-11-14T19:14:00Z

definitions/procedures/packages/update_all_confirmation.rb

+    metadata do
+      param :packages, 'List of packages to update', :array => true
+
+      description 'Confirm update all is intentionall'


s/intentionall/intentional/

@gnurag thanks, updated.

kgaikwad

+1 for hashsyntax style no_mixed_keys.

kgaikwad · 2019-12-09T07:27:33Z

@upadhyeammit, @gnurag, any additional comments?

@jameerpathan111, would you like to do final round of testing with updated changes?

upadhyeammit · 2019-12-09T09:28:44Z

@upadhyeammit, @gnurag, any additional comments?

@jameerpathan111, would you like to do final round of testing with updated changes?

Nothing else from my side.

jameerpathan111 · 2020-01-13T12:20:40Z

@jameerpathan111, would you like to do final round of testing with updated changes?

@kgaikwad tested this PR and it works as expected.
Observation:

able to update all packages using foreman-maintain packages update
--assumeyes for this specific scenario is disabled, user will still have to manually confirm if he wants to update all packages.( reason Fixes #28097 - Allow packages update with no packages #286 (comment) )
- Except when it asks for permission to run satellite-installer.

This is how output of command now looks like:

# foreman-maintain packages update --assumeyes 
Running preparation steps required to run the next scenarios
================================================================================
Check if tooling for package locking is installed:                    [OK]
--------------------------------------------------------------------------------


Running update packages in unlocked session
================================================================================
Confirm update all is intentional: 

WARNING: No specific packages to update were provided
so we are going to update all available packages.
It is recommended to update everything only as part of upgrade
of the Satellite to the next version. 
To Upgrade to next version use 'foreman-maintain upgrade'.

NOTE: --assumeyes is not applicable for this check

Do you want to proceed with update of everything regardless
of the recommendations?, [y(yes), q(quit)] y
                                                                      [OK]      
--------------------------------------------------------------------------------
Confirm installer run is allowed: 

WARNING: This script runs satellite-installer after the yum execution 
to ensure the Satellite is in a consistent state.
As a result some of your services may be restarted. 

Do you want to proceed? (assuming yes)   ---> Here assumeyes is working.
                                                                      [OK]      
--------------------------------------------------------------------------------
Unlock packages:                                                      [OK]
--------------------------------------------------------------------------------
Update package(s) :                                                   [OK]
--------------------------------------------------------------------------------
==============================================================================================================================================================
 Package                                           Arch   Version                          Repository                                                    Size
==============================================================================================================================================================
Updating:
*****************************
****  Output omitted   ****
*****************************
foreman-rake upgrade:run finished successfully!
Upgrade completed!
Package versions are being locked.
       [OK]
--------------------------------------------------------------------------------
Check status of version locking of packages: 
  Automatic locking of package versions is enabled in installer.
  Packages are locked.                                                [OK]
--------------------------------------------------------------------------------

kgaikwad · 2020-02-10T08:18:01Z

Thank you @jameerpathan111 for testing this PR.
Merging this pull-request..

kgaikwad · 2020-02-10T08:21:33Z

Thank you @mbacovsky 🎉 👌

Thanks @upadhyeammit, @gnurag for reviewing this PR

Fixes #28097 - Allow packages update with no packages

a706762

This patch lets packages update command to be invoked without any args to update all packages.

theforeman-bot added Needs testing Not yet reviewed labels Oct 21, 2019

Refs #28097 - Confirm update all

ae1454d

Refs #28097 - rubocop

4262887

Refs #28097 - updated formatting of the message

7d34057

mbacovsky force-pushed the 28097-update_all branch from f180c62 to 7d34057 Compare November 5, 2019 11:56

Refs #28097 - Disable --assumeyes

83e3370

gnurag suggested changes Nov 14, 2019

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Nov 14, 2019

Refs #28097 - fixed typo

1a654bc

theforeman-bot added Needs re-review and removed Waiting on contributor labels Nov 15, 2019

kgaikwad approved these changes Nov 17, 2019

View reviewed changes

theforeman-bot removed the Needs re-review label Nov 17, 2019

upadhyeammit approved these changes Dec 9, 2019

View reviewed changes

kgaikwad added the 0.5.x pull-requests targeted for next version 0.5.x(downstream) label Jan 9, 2020

kgaikwad merged commit b6b2581 into theforeman:master Feb 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #28097 - Allow packages update with no packages #286

Fixes #28097 - Allow packages update with no packages #286

mbacovsky commented Oct 21, 2019

theforeman-bot commented Oct 21, 2019

ntkathole commented Oct 21, 2019

mbacovsky commented Oct 21, 2019

upadhyeammit commented Oct 22, 2019

kgaikwad commented Oct 22, 2019

mbacovsky commented Oct 30, 2019

kgaikwad commented Oct 30, 2019

mbacovsky commented Oct 30, 2019

jameerpathan111 commented Oct 31, 2019

mbacovsky commented Nov 5, 2019

jameerpathan111 commented Nov 6, 2019

kgaikwad commented Nov 6, 2019

jameerpathan111 commented Nov 11, 2019

mbacovsky commented Nov 11, 2019

jameerpathan111 commented Nov 12, 2019

mbacovsky commented Nov 14, 2019

gnurag Nov 14, 2019

mbacovsky Nov 15, 2019

kgaikwad left a comment

kgaikwad commented Dec 9, 2019

upadhyeammit commented Dec 9, 2019

jameerpathan111 commented Jan 13, 2020

kgaikwad commented Feb 10, 2020

kgaikwad commented Feb 10, 2020

Fixes #28097 - Allow packages update with no packages #286

Fixes #28097 - Allow packages update with no packages #286

Conversation

mbacovsky commented Oct 21, 2019

theforeman-bot commented Oct 21, 2019

ntkathole commented Oct 21, 2019

mbacovsky commented Oct 21, 2019

upadhyeammit commented Oct 22, 2019

kgaikwad commented Oct 22, 2019

mbacovsky commented Oct 30, 2019

kgaikwad commented Oct 30, 2019

mbacovsky commented Oct 30, 2019

jameerpathan111 commented Oct 31, 2019

mbacovsky commented Nov 5, 2019

jameerpathan111 commented Nov 6, 2019

kgaikwad commented Nov 6, 2019

jameerpathan111 commented Nov 11, 2019

mbacovsky commented Nov 11, 2019

jameerpathan111 commented Nov 12, 2019

mbacovsky commented Nov 14, 2019

gnurag Nov 14, 2019

Choose a reason for hiding this comment

mbacovsky Nov 15, 2019

Choose a reason for hiding this comment

kgaikwad left a comment

Choose a reason for hiding this comment

kgaikwad commented Dec 9, 2019

upadhyeammit commented Dec 9, 2019

jameerpathan111 commented Jan 13, 2020

kgaikwad commented Feb 10, 2020

kgaikwad commented Feb 10, 2020