coreos
Butane merge
Again, got caught off guard by GH defaulting to PRs on the original repo not the forked one
This PR implements one of the idea proposed in #118 (stale for ~1 year), hoping to start discussions again. This, combined with #629 opens up so much possibilities !
The idea I implemented is adding inline_butane and local_butane directives. These are converted to ignition format when parsed.
Note: These use the same filesDir as the main butane file. This might not be the desired behavior (was discussed in #118) I feel like the current proposed change can accommodate a new files-dir relatively easily. Feel free to share your thoughts!
Note2: I'm a. bit unsure about the docs I should write, or how to make fields mutually exclusive
Disclaimer: This is my first time working with Go. I'm learning as I go, so things might not be perfect or fully optimized. Please be kind 🙏
I have 2 versions of the same feature (you can see the second version here: https://github.com/vic1707/butane/pull/3).
Both versions do essentially the same thing but circumvent the cyclic import problem I encountered when trying to translate the imported butane to ignition. I have no opinions on which is better nor do I know if there's a better way 😓.
Been experimenting with the feature (alongside gomplate integration) it's really great to be able to split butane. (See my experiments here if you want https://github.com/vic1707/homelab-config/pull/52)
But the inheritance of the filesDir can quickly become a pain, I'll reread https://github.com/coreos/butane/issues/118 and implement one of the idea (separate branch in case you don't want both at the same time)
Added the possibility to use a new custom files-dir for imported butanes: https://github.com/vic1707/butane/pull/4 (only a local PR for now if the solution proposed is accepted)
Thank you @vic1707 for submitting this, I wanted to reach out to let you know I am aware and am planing to take a look sometime soon.
Thanks for the review and the time you're giving me, if you are experimenting with the changes you might also want to take a look at https://github.com/vic1707/butane/pull/4 which allows for an overwrite of the files-dir for imported butane configs (it really convenient when managing multiple root butane configs)
This is a great addition, I hope it won't go stale
Hope so too, if you're able to test/review/hint me a little on how to improve things I'd greatly appreciate !
@janus-reith / @vic1707
Its entirely on me, I am so sorry about the slow turn around on this; I will make this my priority now @vic1707.
Okay, so I finally got around to testing this locally. It works as described for simple cases and is suuuper cool (1 , 2 layers of merge).
Two major points of pain: This does open up a potential stack overflow, if you merge the same config .. i.e (self-reference.bu)
variant: fcos
version: 1.7.0-experimental
ignition:
config:
merge:
- local_butane: self-reference.bu
I am not sure how to identify this at runtime, and it would only effect the user running it but there are ongoing talks about being able to run from butane configs which could change our risk scope a bit.
Additionally this could happen accidentally to something like a.bu → b.bu → c.bu → a.bu
I think to prevent this best, we could have a depth limit? maybe 10 bonus points if its configurable (not butane file but argument into butane?
The cycle issue:
Once we translate reference our self's .. bam cycle.
┌─────────────────┐
│ base/v0_7_exp │ ← Contains schemas AND translation logic
│ │
│ - schema.go │ ← Data structures (Resource, Config, etc.)
│ - translate.go │ ← Translation logic (ToIgn3_6Unvalidated)
└─────────────────┘
↑
│ imports (for schemas)
│
┌────────────────────┐
│ config/fcos/v1_7 │ ← Variant implementation
│ │
│ - schema.go │ ← Wraps base schemas
│ - translate.go │ ← Calls base.ToIgn3_6Unvalidated()
└────────────────────┘
↑
│ imports (init registers translators)
│
┌────────────────────┐
│ config/config │ ← Translator registry
│ │
│ - registry map │ ← Maps variant→translator
│ - TranslateBytes │ ← Public API
└────────────────────┘
↑
│ WANTS TO IMPORT (for nested translation)
│
┌─────────────────────┐
│ base/v0_7_exp │
│ translateButane... │ ← Needs to call TranslateBytes()
└─────────────────────┘
CYCLE: base → config → fcos → base
-
The way you solved it in this pr definitely works but I think we can do it a different way. Because the current way introduces public functions unnecessarily. Additionally makes things a little confusing working in the code.
-
The reflection PR also would work but kinda fragile and could be a pain to work with.
My thoughts are that we need to add a layer of abstraction to avoid this. This would invert the dependency and should get us around the cycle problem. We should be able to do this with interfaces, i.e no longer directly reference implementation but the interface.
This can happen external to this PR and probably should. I can create a PR which can do this but I think we should go this route.
New Structure:
translate/ ← NEW: Top-level package
interface.go ← Define Translator interface
registry.go ← Registry implementation
base/v0_7_exp/
schema.go ← Data structures only
translator.go ← Implements translate.Translator interface
config/fcos/v1_7_exp/
schema.go ← Wraps base schemas
translator.go ← Implements translate.Translator interface
init.go ← Registers with translate.Registry
Hi, Thanks for the review and tips!
Kinda never thought of recursive calls. good catch! I guess it's possible to identify them as (IIRC) we pretty much always have access to the currently read butane file + path, storing the ones we visited and looking for a match should be doable. Else a recursion limit argument is a good fallback.
Thanks for the proposed solution to the circular imports issue, I like it way more than my 2 current propositions 👍. Moving files around seems like a simpler solution albeit more time consuming to implement. Doing it in another PR seems obvious to me.
If you think someone not that familiar with the project can pull it off I'd like giving it a go, hopefully this weekend (if I can free some time), but if you prefer doing it yourself, please be my guest 😁.
No matter what I think tackling the circular import si the first thing that needs to be done here. (I haven't forgot the API/docs issues you mentioned in your first quick review)
Lol when I realized I was like wait a moment.. can i... and yes I could
Of course. I have no doubt you could pull it off, if you want to give it a try, open a draft PR, I can help you in your branch if you get stuck. Its a loooot of linking around once the interfaces are added but the most pivotal point will be defining the interfaces to be appropriate for use.
So I would recommend that we do it in stages,
- define the interfaces.
- review that
- Implement the use of the interfaces
100% I agree, the main point of pain is technical debt, and improving the environment for this feature.
I'll give it a try asap, that sounds like a fun exercise 😁 I just have to finish another PR on one of my repo but after that I can dedicate time to my butane PRs.