-
Notifications
You must be signed in to change notification settings - Fork 4
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
Take global layer into account #351
Conversation
Tested locally using a VM. @rwaffen @bastelfreak @marcusdots any feedback from your side? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't take a very close look now, but it looks okay to me. There's a lot of restructuring going on, so there's a lot of movement in the code. But since the removed and added lines roughly match up, it should be fine. If Martin tested it and it's functional, I don't see any problem merging it.
This PR is a good reason for a major release: v3.0.0 |
i merged the dependabot and we merge this we have all open PRs done. nice. then lets do a release! :-D |
Maybe the big 3.0 release should wait for #331 (and maybe #340 as well)? I will start working on the latter today. I have not looked into the module layer thing much, but the basis is already here, so I guess it is more a matter of figuring out when to access which module. Should not be too hard (i know, i know, "famous last words" 🙂 ). |
"how hard can that be.... oh..." 😄 |
And take "global" layer into account when querying data.
Fixes the problem displayed here: #330 (comment)
Fixes #330
This is a larger refactoring of the inner workings of hdm to introduce the concept of different "layers" of hiera configuration. As a first step, it allows to show the "global layer" as well as the environment layer that we always supported.
This also paves the way for #331, as recognizing that there is more than one layer is a prerequisite for that.
Having had to move around and change lots of stuff, I decided to change the architecture a little bit. Until now, the models in
app/models
were strictly divided: Top level classes were mostly "high-level" classes that mimicked rails' ActiveRecord-API where possible. The classes inapp/models/hiera_data
represented "low-level" objects that dealt directly with hiera data, files etc.The class
HieraData
inapp/models/hiera_data.rb
was used as the central interface between "high-" and "low-level" classes. It was a nice separation in the sense that the high level classes did not have to know anything about hiera internals. And it has served us well for quite some time. But this also meant thatHieraData
was already a very big and complex class. With the addition of layers this was prone to grow out of hand.Once I decided to break up this structure, it turned out there was a lot of redundant and even some unused code, so I could actually delete a lot of code (always a good thing 🙂). I am really happy with how everything in the "low-level" classes turned out. The change comes with some additional upsides:
HieraData
are being createdBut as always there are downsides as well. The high-level model classes have become more complex as they need to know more about the hiera internals. They also have more dependencies, because more references to low-level classes need to be passed around. I am not 100% happy with the current state here, as it feels more "bolted on" than actually designed. But I think that #331 and/or #84 may bring more clarity here.
There is also a small detail that changed because of these refactorings and that is not trivial to change back: When doing a key search, hdm now displays the full path to the files instead of a shorter path relative to the
datadir
:I hope that is OK. If not, I think I can find a way to restore the old behavior. It just will not be pretty 😬
Please note that given how big a change this is, this might introduce some instability. Things that used to work might be broken so give this a good test-drive if possible.
One thing I left out intentionally is the API / integration with foreman. I made sure the behavior of the API did not change, but did nothing more. If we want the global layer in foreman as well, we should create a new issue for that (and decide if that warrants and API v2). But this should probably wait until after #331.