Fixes #20821 - Separate katello-* utilities into classes

[johnpmitsch]

This moves katello-backup, restore, and hostname change into classes in lib/ that will be called by the actual scripts. This allows us to have more flexibility in changing terminology in backports and also makes the script functionality more testable and compartmentalized.

TO-DO:

• [x] update spec files

[theforeman-bot]

Issues: #20821

[johnpmitsch]

To test, spin up a 3.4 box and run:

mkdir /usr/share/katello
cd ~
git clone https://github.com/Katello/katello-packaging
cd katello-packaging/
git remote add fork https://github.com/johnpmitsch/katello-packaging
git fetch --all
git checkout fork/katello_script_refactor
ln -s /root/katello-packaging/katello/lib/restore.rb /usr/share/katello/restore.rb
ln -s /root/katello-packaging/katello/lib/backup.rb /usr/share/katello/backup.rb
ln -s /root/katello-packaging/katello/lib/hostname-change.rb /usr/share/katello/hostname-change.rb
ln -s /root/katello-packaging/katello/lib/script-helper.rb /usr/share/katello/script-helper.rb
cd ~
ln -s /root/katello-packaging/katello/katello-change-hostname katello-change-hostname
ln -s /root/katello-packaging/katello/katello-restore katello-restore
ln -s /root/katello-packaging/katello/katello-backup katello-backup
chmod a+x katello-change-hostname 
chmod a+x katello-restore
chmod a+x katello-backup

All backup/restore/change-hostname functionality should be the same as usual

[jturel]

Just starting my own review but I'm curious if you see any value in one or more of these options:

• Putting the Backup script into module => Katello::Backup.new
• Renaming Backup to BackupCreator which I think makes the intent of the class more clear

Ditto on those suggestions for the other scripts you adjusted.

[johnpmitsch]

@jturel I am for having a namespace to make things clear, but one thing I do want to avoid is having a useless subdirectory, i.e. katello/lib/katello/backup.rb vs katello/lib/backup.rb. I think I can add the modules without the subdirectory (I don't think this breaks conventions or best practices), I'll do that.

Let me think about the names a little more, one idea I have is this:

Tools::Backup
Tools::Restore
Tools::HostnameChange

or

Utilities::Backup
Utilities::Restore
Utilities::HostnameChange

That would signify that these are tools and not actual backups, etc. I'm not sure if it needs a specific Katello declaration (since we aren't in a plugin situation like we are in actual Katello repo), but open to it if others agree. \

[jturel]

I was able to test each of the scripts and see that they are working as intended! I'll accept once the module is renamed. Deferring to Eric on the spec file changes (though, they make sense).

[ehelms]

@johnpmitsch What's the plan here? I see a lot of fixes going into some of these scripts thus causing this to need a rebase and update. Which will lead this to need to be re-tested.

[johnpmitsch]

@ehelms I have to figure that out, its probably going to be that I manually apply all of the recent commits to the katello-change hostname class

[johnpmitsch]

I updated this with the latest katello-change-hostname commits and renamed the Utitlies class to KatelloUtilities. @evgeni Can you make sure all of the latest commits to katello-change-hostname are found in here? I manually applied them since I couldn't rebase properly with the refactoring. @ehelms Can you check the spec file? @jturel Can you retest? Here is what an updated version of what I use to test

yum install -y git vim
mkdir /usr/share/katello
cd ~
git clone https://github.com/Katello/katello-packaging
cd katello-packaging/
git remote add fork https://github.com/johnpmitsch/katello-packaging
git fetch --all
git checkout fork/katello_script_refactor
ln -s /root/katello-packaging/katello/utilities/restore.rb /usr/share/katello/restore.rb
ln -s /root/katello-packaging/katello/utilities/backup.rb /usr/share/katello/backup.rb
ln -s /root/katello-packaging/katello/utilities/hostname-change.rb /usr/share/katello/hostname-change.rb
ln -s /root/katello-packaging/katello/utilities/helper.rb /usr/share/katello/helper.rb
cd ~
ln -s /root/katello-packaging/katello/katello-change-hostname katello-change-hostname
ln -s /root/katello-packaging/katello/katello-restore katello-restore
ln -s /root/katello-packaging/katello/katello-backup katello-backup
chmod a+x katello-change-hostname 
chmod a+x katello-restore
chmod a+x katello-backup

[jturel]

I retested all scripts and they are working well.

[johnpmitsch]

After some off-thread IRC conversations, I updated the scripts to live in the katello/ dir, everything. I'm currently re-testing the scripts and so far, so good.

@jturel apologies, but do you mind retesting? You will just have to update the links in the setup script that I put in a previous comment. (thanks for being patient)

@ehelms I'm not sure what is going wrong with the build in jenkins, but I suspect something is off in my spec file. Can you take a look? I can't seem to get the exact error (The output mentions checking build.log).

[mbacovsky]

The build is green now, the rpm can be downloaded from the task [1] for easier testing. [1] http://koji.katello.org/koji/taskinfo?taskID=29919

[jturel]

Everything is still good with the change to the KatelloUtilities namespace. 👍

[johnpmitsch]

@jturel @evgeni Can you double check that all the changes that have been made since this PR has been open are correctly included in this PR? I wasn't able to resolve any merge conflicts since the files moved and changed so drastically, so applied them manually.

https://github.com/Katello/katello-packaging/commits/master/katello/katello-change-hostname

[johnpmitsch]

Updating to use accept an argument for the accepted_scenarios (again, for backporting). Changes are in a separate commit

This will allow us to overwrite the accepted_scenarios method, as shown below:

module Foo
  def scenarios
    @accepted || ["in module"]
  end
end

class Bar
  include Foo

  def initialize(accepted=nil)
    @accepted = accepted
  end
end

2.2.4 :002 > bar_no_params = Bar.new
 => #<Bar:0x00000000b1dcd0 @accepted=nil> 
2.2.4 :003 > bar_no_params.scenarios
 => ["in module"] 
2.2.4 :004 > bar_with_params = Bar.new(["not in module"])
 => #<Bar:0x00000000a8cb90 @accepted=["not in module"]> 
2.2.4 :005 > bar_with_params.scenarios
 => ["not in module"] 

[johnpmitsch]

Testing out the accepted scenarios changing:

diff --git a/katello/katello-backup b/katello/katello-backup
index 745143e..f74b3c8 100755
--- a/katello/katello-backup
+++ b/katello/katello-backup
@@ -1,7 +1,7 @@
 #!/usr/bin/env ruby
 require "/usr/share/katello/backup.rb"

-katello_backup = KatelloUtilities::Backup.new("foreman-proxy-content", "foreman proxy")
+katello_backup = KatelloUtilities::Backup.new("foreman-proxy-content", "foreman proxy", ["blah"])

 if katello_backup.accepted_scenarios.include? katello_backup.last_scenario
   katello_backup.run
diff --git a/katello/katello-change-hostname b/katello/katello-change-hostname
index b5029c0..e4d5f8d 100755
--- a/katello/katello-change-hostname
+++ b/katello/katello-change-hostname
@@ -1,7 +1,7 @@
 #!/usr/bin/env ruby
 require "/usr/share/katello/hostname-change.rb"

-hostname_change = KatelloUtilities::HostnameChange.new("Foreman Proxy", "Foreman Proxies", "foreman-proxy-content")
+hostname_change = KatelloUtilities::HostnameChange.new("Foreman Proxy", "Foreman Proxies", "foreman-proxy-content", nil, nil, ["blah"])

 if hostname_change.accepted_scenarios.include? hostname_change.last_scenario
   hostname_change.run
diff --git a/katello/katello-restore b/katello/katello-restore
index e0f7bca..c3fce8f 100755
--- a/katello/katello-restore
+++ b/katello/katello-restore
@@ -1,7 +1,7 @@
 #!/usr/bin/env ruby
 require "/usr/share/katello/restore.rb"

-katello_restore = KatelloUtilities::Restore.new("foreman-proxy-content")
+katello_restore = KatelloUtilities::Restore.new("foreman-proxy-content", ["blah"])

 if katello_restore.accepted_scenarios.include? katello_restore.last_scenario
   katello_restore.run

[root@change-hostname-34 katello]# ./katello-backup /backup
This utility can't run on a non-katello system.
[root@change-hostname-34 katello]# ./katello-restore /backup
This utility can't run on a non-katello system.
[root@change-hostname-34 katello]# ./katello-change-hostname -u admin -p changeme -d foo.example.com
This utility can't run on a non-katello system.

An error message is displayed when you pass in a parameter that is not an accepted scenario to the classes

[jturel]

@johnpmitsch All the porting of recent changes (from 9-14 to present) look good!

[johnpmitsch]

@ehelms can you look over the recent changes?

[johnpmitsch]

@evgeni This is meant to be a "step in the right direction" change to start the utilities moving over to foreman-maintain. I am not sure I would want to change this to be a gem before doing that, I plan to start on the effort after this gets merged.

I agree on the further DRYing up of the classes, but wanted to keep things simple in this first refactoring and then follow up with smaller additional changes (moving other scripts into classes and removing duplicate commands). I would rather merge this first so any additional changes anyone makes to the three refactored scripts can use this structure and I don't have to keep manually applying changes to master to this PR. Of course, as always, I'm open to discussion :)

[johnpmitsch]

@evgeni I updated according to your suggestions, can you give it a second look?

[ehelms]

@johnpmitsch I merged your man-page PR and I think I broke the spec file. Please rebase.

[ehelms]

@evgeni As stated, the general agreement and discussion was to do this a as a step towards foreman_maintain, without going full scale in creating a new gem, new packaging, new everything. While this will likely persist a full release, I don't agree with breaking it out at this time. Yes, it makes this place uglier, but the intent was to do as little as possible to re-factor into re-usable classes to facilitate easier re-use and a shortened timescale. Another way to look at it is, we want to fix the "bug" not turn this into a feature.

I should also note that I agree with @johnpmitsch in that, in this case, re-factoring here will lead to higher risk rather than lower risk due to the amount and complexity of changes that are introduced. By first wrapping existing functionality into classes with as few new methods as possible we retain the original behaviors enough to regression test easier. The more that changes, the more edge cases we potentially introduce on untested code.

The ideal move to foreman_maintain will introduce rubocop, and testing and a singular framework for use which will provide a lot more robustness.

[ehelms]

Looks like yet again a round of re-factors has occurred meaning this will need the full gambit of re-testing. Does that fall to you @jturel ?

[johnpmitsch]

@ehelms thanks, I rebased the man page, please double check it looks ok. @evgeni is going to take another look at the latest refactor

@jturel I would recommend holding off on testing until @evgeni takes a look and comments, that way we can save you some unnecessary testing if there are additional changes :)

[johnpmitsch]

@evgeni nice catch! I updated @jturel lets test this out again (hopefully for the last time)

[jturel]

I was able to verify all scripts against a 3.4 installation and a proxy!