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

Enable nested profiles #1572

Closed
wants to merge 1 commit into from
Closed

Conversation

griznog
Copy link
Contributor

@griznog griznog commented Nov 30, 2024

  • Move Profiles from Node struct to Profile struct.
  • Enable/handle nested profiles.
  • Add test for merged profile tags.
  • Update CHANGELOG.md

Description of the Pull Request (PR):

Write your description of the PR here. Be sure to include as much background,
and details necessary for the reviewers to understand exactly what this is
fixing or enhancing.

This fixes or addresses the following GitHub issues:

  • Fixes #

Reviewer checklist

The reviewer checks the following items before merging the PR.

  • The PR is based on the appropriate branch (typically main)
  • All commits are "Signed off" (e.g., using git commit --signoff) in agreement to the DCO
  • The CHANGELOG has been updated, if necessary, and under the correct release heading
  • The userdocs have been updated, if necessary
  • The submitter is listed in the contributors file
  • The test suite has been updated, if necessary

@griznog
Copy link
Contributor Author

griznog commented Nov 30, 2024

When I make test there are a lot of failing tests, some of which look like they might be related to moving Profiles from the Node struct to the Profile struct, but a whole lot more that don't seem to be related. I must shamefully admit that today is the first time I've ran make test locally so I don't know how normal the failed tests are or if they are just something I have broken locally on my system.

@anderbubble let me know if my test is ok, I think I want to add some more checks to it around tags/values in netdev and ipmi, but wanted to start simple.

@mslacken
Copy link
Member

mslacken commented Dec 2, 2024

Using the profile name as sorting order sound arbitrary and I don't like the idea. Still it might be the best solution as

  1. users can change the order by renaming the profile, like in init.d
  2. its predictable all the time
    @anderbubble what's your take?

@griznog
Copy link
Contributor Author

griznog commented Dec 2, 2024

@mslacken I played a little with the idea of building a graph and merging profiles in the order they are specified/included, but eventually settled on the approach of flattening the list of profiles and sorting as a compromise that makes it easy to understand how the merging is done and easier to control the order in the case someone wants to do that.

In my local WW foo I've found that both overlays and profiles work much better and much more understandably if I stick to the best practice of never setting the same value in multiple profiles and never letting two included overlays touch/modify the same file. The further I go down the profile/overlay rabbit hole the more I lean toward detecting either of those things happening and throwing an error or at least a warning when it does happen as it, at least in my experience, always leads to confusion and regret when it happens.

@griznog
Copy link
Contributor Author

griznog commented Dec 2, 2024

This still needs some work. Tags merge correctly but overlays are not appending correctly, presumably mergo is not doing what I think it is doing.

@anderbubble anderbubble marked this pull request as draft December 2, 2024 22:32
@griznog
Copy link
Contributor Author

griznog commented Dec 3, 2024

Figured out the issue with overlays so this currently works w.r.t. properly merging profiles and overlays included in profiles.

It looks like moving Profiles into the Profile struct has some side effects for wwctl node list. Not clear to me if this is a bad thing, in my mild probing I think it just means wwctl node list now shows the profiles.

I can update the failing tests so they pass with the new structure, but want to make sure that is the correct solution before I start doing that.

@anderbubble anderbubble changed the title Enable nested profiles. Enable nested profiles Dec 11, 2024
@anderbubble anderbubble added this to the v4.6.0 milestone Dec 11, 2024
1. Move Profiles from Node struct to Profile struct.
2. Recursively include nested profiles in profiles, eliminating
   duplicates and loops.
3. Sort the profile list alpanumerically
4. Merge such that later profiles overwrite values in earlier
   profiles, with the node config being the last thing merged.
@anderbubble anderbubble force-pushed the nested-profiles branch 2 times, most recently from d33e57a to 65c413a Compare December 13, 2024 02:28
@anderbubble anderbubble mentioned this pull request Dec 18, 2024
6 tasks
@anderbubble
Copy link
Collaborator

Closing in favor of #1598

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.

3 participants