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

Host Registration - Install packages #8298

Merged
merged 1 commit into from Apr 19, 2021

Conversation

stejskalleos
Copy link
Contributor

@stejskalleos stejskalleos commented Jan 29, 2021

New host macro :manage_packages for package installation
on supported OS families: Red Hat, Debian & Suse.

In host registration users can now install packages
on the host by setting the host_registration_packages.

@theforeman-bot
Copy link
Member

Issues: #31743

@stejskalleos
Copy link
Contributor Author

@ekohl updated & rebased
Removed manage_packages macro, now just using snippet_if_exists.
Added dnf support for Fedora and dropped support for SUSE (Global Registration don't support it yet).

@ares
Copy link
Member

ares commented Feb 4, 2021

It seems we ship the snippet and only activate it if host param is defined. In such case, we should use snippet instead of snippet_if_exists. I think the idea with macro wasn't bad, it was the combination with the snippet that caused confusion. I think we should provide some reasonable way to install package (macro or snippet) and then, use it here. That template has already more logic, e.g. for suse.

So I wonder, would it be better to take the content of the linked job template and convert it to macro in core? No snippet involved. The package list is based on the parameter. The only downside is, user can't extend it without Foreman code core change.

Also we could then use the same macro in our provisioning templates, where we today use snippet_if_exists logic, which was probably the motivation to do it the same way here.

@ares
Copy link
Member

ares commented Feb 4, 2021

Given the above, next steps:

  1. @ekohl @stejskalleos do we prefer macro or snippet? I vote for macro
  2. @stejskalleos would it be possible to base the implementation on the linked job template and modify that afterwards so it uses internally macro/snippet? I think that would payback long term

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that in principe a macro is a good idea, but I did struggle with the use of a snippet. We are providing an API with it. How clear is it to the user that if they make changes to that template, they can change the API?

One thing to consider: the commit message says "after host registration" but it calls built after installing packages. Perhaps during instead of after is a better description?

I would also consider solving this in a more generic way: don't include a parameter by default but give the user a generic snippet_if_exists 'registration_linux_post' so they can customize their registration via their own snippet.

@ares
Copy link
Member

ares commented Feb 5, 2021

Ok, so to summarize

  1. let's add the macro, no snippet involved, based on the code in job template
  2. let's use this macro to install packages specified in the parameter called e.g. additional_packages in linux registration template
  3. ideally add the same thing to the kickstart templates
  4. add <%= also snippef_if_exists 'registration_linux_post' %> to the linux registration template, so users can further customize the registration without modifying the default template
  5. let's modify the rex job template to use that macro (that's rex repository work)

please speak up if I misunderstood

@ekohl
Copy link
Member

ekohl commented Feb 5, 2021

ideally add the same thing to the kickstart templates

In kickstart you have 2 types of package installation: the native %packages section which also gives you a nice progress bar. However, it's generally not usable on RHEL due to subscriptions (though since RHEL 8.2 Anaconda understands activation keys so there it can be done, our kickstarts don't understand this).

I'd expect preseed to have something similar.

I'd be ok with leaving it out of scope for now.

Other than that 👍

@stejskalleos
Copy link
Contributor Author

@ares @ekohl
Updated according to #8298 (comment)

Copy link
Member

@ares ares left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I'd just wait for the renaming PR to get in and rename things here so we don't have to rename again after merge.

@stejskalleos stejskalleos changed the title Fixes #31743 - Install packages after host registration [WIP] Fixes #31743 - Install packages after host registration Mar 1, 2021
@stejskalleos stejskalleos changed the title [WIP] Fixes #31743 - Install packages after host registration Host Registration - Install packages Mar 15, 2021
@stejskalleos stejskalleos force-pushed the ls/registration_pkgs branch 2 times, most recently from 8147f51 to 99d74c9 Compare March 25, 2021 12:13
@stejskalleos
Copy link
Contributor Author

@ekohl @ares Rebased & updated

Added banner, Fedora & RHEL >=8 use dnf, fixed condition for subman on debian.

@stejskalleos stejskalleos force-pushed the ls/registration_pkgs branch 2 times, most recently from 513c9cf to 60f175c Compare March 25, 2021 13:46
Copy link
Member

@ares ares left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getting closer and closer :-) two small things and I'll test the whole thing

For package installation on supported OS families: Red Hat, Debian & Suse.
In host registration users can now install packages
by setting the `host_registration_packages` parameter.
Copy link
Member

@ares ares left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested the functionality and all works fine including the snippet. Leaving this open for a few days or until other reviews confirm concerns are addressed. If I don't see any reaction I plan to merge this by the end of the week.

@ares ares merged commit bf56104 into theforeman:develop Apr 19, 2021
@ares
Copy link
Member

ares commented Apr 19, 2021

Thanks @stejskalleos, merged and updated the commit message to reflect the new names

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants