Skip to content
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

Restructure classes #331

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

bastian-src
Copy link

@bastian-src bastian-src commented Aug 11, 2020

Restructure the script and implement classes for Debian/Ubuntu and SUSE integration

  • Introduce function get_os to determine current OS (using /etc/os-release or the platform library as backup)
  • Create abstract class OSHandler
    • Move general functions into OSHandler (functions that don't use redhat specific paths/commands)
  • Create class OSHandlerRHEL (implementing OSHandler)
    • Move OS specific functions into OSHandlerRHEL
  • In __main__, distinguish between current OS and instantiate responding OSHandler implementation
  • Create class OSHandlerSLES (implementing OSHandler)
    • Implement OS specific functions
  • Create class OSHandlerDeb (implementing OSHandler)
    • Implement OS specific functions
  • Remove Python 2.5 support (and Centos5 test) for better exception handling

@sbernhard
Copy link
Member

ping @evgeni :-)

@evgeni
Copy link
Member

evgeni commented Aug 13, 2020

Please don't ping me

@evgeni
Copy link
Member

evgeni commented Aug 25, 2020

So, coming back to this. I don't think we should spend too much time on the current version of bootstrap.py as there is currently movement to revamp the whole workflow and ideally replace bootstrap.py with a tiny script that just pulls and executes templates from Foreman and also includes support for non-RHEL systems.

@chris1984
Copy link
Member

Why do we have to have objects and classes, can we just use one long function with if statements?

@bastian-src
Copy link
Author

Let's assume we go with one long function and if statements, there will be (kind of the same) statement every time we want to differ between Debian, RHEL and SLES. This leads to duplicated code.
When we use classes, this choice must be made only once at the beginning. Afterwards, the corresponding class, which implements the OS specific calls (yum, zypper, apt) and configurations, is used. I suggest an abstract class OSHandler that implements the general procedure of the script and defines some empty function bodies like pm_install(package_name) (package manager install) or setup_repo(url, gpg). Then, the classes OSHandlerRHEL, OSHandlerSLES, OSHandlerDeb inherit from OSHandler and implement the specific functionalities.

This approach has no duplicated code and increases readability by grouping OS specific implementations under one class.

@chris1984
Copy link
Member

@bastian-src thanks for explaining it to me, I am still trying to get a grasp on OOP so that helps!

@bastian-src bastian-src force-pushed the restructure_classes branch 2 times, most recently from 2df8ad6 to 148604b Compare September 28, 2020 12:40
@bastian-src bastian-src force-pushed the restructure_classes branch 2 times, most recently from d1db120 to b12c3be Compare January 22, 2021 13:55
Bastian Schmidt and others added 3 commits January 25, 2021 15:32
* Move general functions into OSHandler
* Integrate OSHandlerRHEL which implements OSHandler
* Move RHEL specific functions into OSHandlerRHEL
* Remove Python 2.5 support
* Support SLES11 SP4, SLES12 SP5, SLES 15 /SP1/2
* Identify abstract, private and public functions for OSHandler
* Introduce _pm* functions for OS specific package manager calls
* Create general functions blank_migration, capsule_migration,
  classic_migration and clean_and_update
* Add python-rpm dependency for SLES
* Reimplement _check_package_version as function of OSHandler
* Reimplement check_package_installed as function of OSHandler
* Add function to express mandatory Python module imports
* Add output suppression for exec_command, exec_failok, and exec_failexit
* Implement _check_package* functions with system calls
* Implement Debian specific calls in _setup_repo, _install_client_ca and
  install_prereqs
* Add dynamic base url due to varying repo paths
* Verify Ubuntu support of bootstrap.py
@bastian-src bastian-src force-pushed the restructure_classes branch 2 times, most recently from bc17074 to 309ef1c Compare January 26, 2021 15:07
@B3DTech
Copy link

B3DTech commented Mar 29, 2021

There is a problem with this. You clear out the apt sources.list file before installing the subscription manager packages, specifically python3-subscription-manager, which means that it is not able to install the prerequisite packages and the script fails.

@bastian-src
Copy link
Author

@B3DTech Thanks for your reply! I appreciate that someone is having a look at this PR.

On Debian/Ubuntu, the subscription-manager package is usually not included in the default repositories. Therefore, you have to pass the --deps-repository-url and --deps-repository-gpg-key parameters when running the script (providing an alternative repo).
If you do so, in line 1681, during _install_prereqs, these two arguments will be passed to _setup_repo which can be seen in line 1534. At this point, the sources.list is "cleared" as you mentioned, but the additional repo is added afterwards which shall provide all dependencies to install subscription-manager.

Have in mind, both parameters (-url and -gpg-key) contain URLs. The script downloads the gpg key from the given URL and runs apt-key add ... on the retrieved file.

@chris1984
Copy link
Member

What are your thoughts on rewriting this entire tool in Ruby?

@B3DTech
Copy link

B3DTech commented Mar 30, 2021

Therefore, you have to pass the --deps-repository-url and --deps-repository-gpg-key parameters when running the script (providing an alternative repo).

I'm sure I'm missing something here coming from the RHEL/CentOS side of things where subscription-manager is built into the repos and I can use --deps-repository-url to specify the CentOS base repo for dependencies.

The --deps-repository-url is supposed to specify the location of the subscription-manager packages. I'm using Aptix repo as it's the only one for Debian/Ubuntu that I can see.

This is supposed to install python3-subscription-manager, which has the following dependencies.

Depends: python3 (<< 3.9), python3 (>= 3.8~), python3-dateutil, python3-ethtool, python3-iniparse, python3-six, python3:any (>= 3.2~), python3-dbus, python3-rpm, virt-what, python3-debian, python-gi, python3-decorator
Suggests: python3-subscription-manager-doc

These dependencies are not available in the Aptix repo, and since the sources.list file with the default Ubuntu repos are removed, there is now no way to resolve these dependencies.

Any thoughts? Thanks!

@bastian-src
Copy link
Author

These dependencies are not available in the Aptix repo, and since the sources.list file with the default Ubuntu repos are removed, there is now no way to resolve these dependencies.

When you are using an Orcharhino, the Orcharhino itself provides you with a repo which holds all dependencies for the bootstrap process. You can see it in the documentation right before the part Using Katello Agent on SLES 11 (compare docs).

So, the script is designed to get all dependencies via an extra repository. As a quick fix you can just put your normal Ubuntu repos in an extra file /etc/apt/source.list.d/ubuntu.list. Let me know if that works for you!

Nevertheless, it can be discussed whether it leads to problems if we remove the sources.list after the installation of the subscription manager instead of doing it before.

@bastian-src
Copy link
Author

What are your thoughts on rewriting this entire tool in Ruby?

If we wanna do exactly the same in Ruby, we would need Ruby to be installed on the host. Then it could also be translated to Ruby, but I think there is also a discussion going on regarding the re-design of this whole process (compare forum).

@B3DTech
Copy link

B3DTech commented Mar 31, 2021

When you are using an Orcharhino

Not everyone uses Orcharhino. In this case it’s Foreman/Katello proper. And since this script is going into the Katello project it should be made universal.

I will modify the bootstrap for my setup as I need this to work but I think this really would impact anyone using Ubuntu or Debian with this script.

@chris1984
Copy link
Member

chris1984 commented Mar 31, 2021

That's fair I am not a fan of Python and Ruby works better. That is why I wanted to switch this to Ruby. We could also do C or C++ which would be native to the OS. With that we could get improved speed too.

@bastian-src
Copy link
Author

When you are using an Orcharhino

Not everyone uses Orcharhino.

Maybe not today and also not tomorrow, but the day will come.

Okay, jokes aside. Of course you are completely right and this script must be unified such that it can work with any foreman installation. Nevertheless, the reason for this single deps repo implementation is the fact that we experienced issues with apt when dependencies were available via multiple sources/repos. That's why other repos in sources.list are disabled before the installation process of the subscription-manager.

Therefore, the usual case should be that the repo, you retrieve your software from, should also satisfy the dependencies for that piece of software. Since this is not always the case, what are your opinions on giving the option to add multiple deps repos? Or maybe just an additional parameter where you can tell the script to disable sources.list after the installation of the subscription-manager?

@B3DTech
Copy link

B3DTech commented Apr 1, 2021

For Unbuntu/Debian - the only source for Subscription Manager is the Atix repository. If there are required pre-req's and versions, a solution would be to have Atix include them in their repo.

The other option would be to have Atix document the additional deb's needed and put it on their apt repo web page. Maybe a quick how-to create that Repo in a native Foreman/Katello install.

Doesn't seem like I can create the repo in Katello/Pulp since I'm using Pulp2 for now and don't have signed repos - although I guess I can delete all my repos, upgrade to pulp3 and re-create them and then manually sign them.

* Don't fail if sub-man-migration can't be installed
* Fix remove rhn-* on OL before subman install
* Add option to only use the deps repository
* Read dnf config to parse proxy configuration
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants