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

Add initial functionality for Cobbler formula #4

Merged
merged 1 commit into from
Jun 17, 2024

Conversation

SchoolGuy
Copy link
Member

This PR should include the basic functionality of the Cobbler Formula:

  • Install the Cobbler package
  • Add the community repository in case Cobbler is not available for your OS (with an explicit flag)
  • Template and manage the settings file
  • Install optional services like DHCP and DNS in case the settings desire it
  • Enable a user to pass an override configuration file that ignores the Salt configuration

Uninstallation Cobbler in a way that no trace is left is not part of this PR. This PR focuses on getting Cobbler running.

@SchoolGuy
Copy link
Member Author

For CentOS 8 we should use https://github.com/saltstack-formulas/epel-formula to enable Cobbler to be used.

@tacerus
Copy link
Member

tacerus commented Oct 25, 2023

I wonder if it might make sense to not template the configuration files which use standard formats, but to rather use the YAML serializer for .yaml configuration failes and ini_manage states for .ini ones. Of course, there might be special cases where custom handling is needed, but from the files I notice in this patch, the only "special" part seems to be the comment block used in the file header.

cobbler/package/clean.sls Outdated Show resolved Hide resolved
@tacerus
Copy link
Member

tacerus commented Oct 25, 2023

Minor nitpick, it would be nice to use {%- instead of {% for Jinja operations in the sls files, as to not print lots of empty lines while running in debug mode.

@SchoolGuy
Copy link
Member Author

SchoolGuy commented Nov 16, 2023

@opoplawski I am trying to get Cobbler installed on CentOS 8 Stream, AlmaLinux 8 & RockyLinux 8. Is the module that is available for CentOS 8 not available for those distros?

Edit 1: Version 9 of the corresponding OSes are sadly not available in their (Saltstack) images repository.

Edit 2: I found the issue. The module is not needed anymore. Now I need to find out why Salt is refusing to install Cobbler. locally without Salt this just works fine.

@SchoolGuy SchoolGuy force-pushed the feature/add-configuration branch 2 times, most recently from b2c1953 to 9f996bb Compare November 17, 2023 11:44
@SchoolGuy
Copy link
Member Author

SchoolGuy commented Nov 17, 2023

@SchoolGuy
Copy link
Member Author

Force push after rebase on main branch.

@SchoolGuy
Copy link
Member Author

SchoolGuy commented Nov 23, 2023

@tacerus Do you have any good idea how to get the installed version of Cobbler and depending on that choose a configuration file? The configuration schema is enforced by Cobbler and for the RHEL 8-based distros we have an older version of Cobbler that differs in the schema. We need a template for each Cobbler version the formula supports and use it appropriately.

The only idea (which wouldn't work out of the box) is that we define the template in the pillar that has to be used and the users need to know which template they need to use. I'm not too fond of that idea, to be honest though.

Edit: I also asked in the Salt Formula Slack since I didn't see any possible inspiration in the official formulas I looked at. - https://saltstackcommunity.slack.com/archives/C7LG8SV54/p1700730778200969

@SchoolGuy
Copy link
Member Author

Executing a command doesn't work as expected since the package is not installed at the time of rendering the template. I need to think more about this...

@tacerus
Copy link
Member

tacerus commented Jan 21, 2024

You could skip the command if the package is not installed, but it's of course not nice having to apply states twice to get the desired result.

I have a far stretched idea: running the command inside the template file used in cobbler-config-file-file-managed, and then {%- include files_switch(...) %} inside there. I think the included template file might only be rendered once cobbler-config-file-file-manage is executed (which already has a require on the package state), unlike the file.sls, which is rendered before the states run. Not sure about this though, haven't tried it.

Edit: on a second thought, this cannot work, as it needs to render it in advance to find a possible diff.

Regarding 9341e73, it might be something for a future patch - you could write the state in pure Python (#!py renderer) allowing you to import and use configparser directly, avoiding having to load a subshell with Python from within Python.

@SchoolGuy
Copy link
Member Author

SchoolGuy commented Jan 21, 2024

@tacerus so in short we have no way to do this in a single run?

Do you think it is viable to ask the user to hardcode the version in the formula? I think not but well I don't have any other idea except running the formula twice...

Running the state twice makes testing in the current CI next to impossible...

@tacerus
Copy link
Member

tacerus commented Jan 21, 2024

It might be possible using an Orchestration state, but I think it would be overkill for this.

I do think setting it using the pillar is viable. What about setting the versions shipped in the distribution packages as defaults?

@SchoolGuy
Copy link
Member Author

Okay this is a great idea. I do think we have a way forward then. I'll get right to it after I am in front of my keyboard.

@SchoolGuy
Copy link
Member Author

@tacerus I have fixed it partly after a good chunk of waiting (lol 9 months). Do you have an idea why the CI fails for CentOS 8 here? The upstream testsuite is successful for CentOS 8 EPEL. Of course, before this is ready for merging I need to do some more fine-tuning.

@SchoolGuy
Copy link
Member Author

With CentOS 9 EPEL installs successfully but the Cobbler module is not available.

@SchoolGuy SchoolGuy added this to the v1.4.0 milestone Jun 14, 2024
@SchoolGuy SchoolGuy requested a review from tacerus June 14, 2024 20:03
@SchoolGuy
Copy link
Member Author

@tacerus I think this PR is now ready for a review from you. It isn't finished but should be good for feedback. After your feedback I will need to verify and fix more OSes then what the CI is running. Lastly I will need to check what templated variables are missing in the different templates.

The CentOS 8 issues are due to CentOS moving its repositories to vault.

@SchoolGuy
Copy link
Member Author

All current parameters from default.yaml should now be utilized to configure Cobbler. Testing in a real-world infrastructure wasn't done yet and validating how good the formula works on other OSes is also an open point. I believe for the scope of this PR the work is done.

@SchoolGuy SchoolGuy marked this pull request as ready for review June 15, 2024 12:27
@tacerus
Copy link
Member

tacerus commented Jun 16, 2024

Hi,

I think this is ok for an initial patch. Some things could be improved, like iterating over the pillar keys instead of declaring all the configuration options individually (to simplify the code but also to guard against unset pillar values).

Testing all the supported distributions would sure be good but can be added over time.

@SchoolGuy SchoolGuy merged commit f037e16 into main Jun 17, 2024
11 checks passed
Copy link

🎉 This PR is included in version 1.4.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants