Butane merge #631

vic1707 · 2025-08-02T11:49:32Z

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: vic1707#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 😓.

gemini-code-assist

Code Review

This pull request refactors the config translation logic by moving generic functionality into a new config/common package. This is a good improvement for code structure and separation of concerns. I've identified two main areas for improvement: the new schema fields InlineButane and LocalButane are introduced without corresponding implementation for translation or validation, which could lead to confusion and unexpected behavior; and an unexported type translator is used in a public API, which is not idiomatic and could be improved for better API clarity. The rest of the changes look solid.

gemini-code-assist · 2025-08-02T11:51:39Z

base/v0_7_exp/schema.go

+	InlineButane *string      `yaml:"inline_butane"`
+	LocalButane  *string      `yaml:"local_butane"`


The new fields InlineButane and LocalButane are added to the schema, but there doesn't seem to be any corresponding logic to handle them in base/v0_7_exp/translate.go or base/v0_7_exp/validate.go. Specifically:

translateResource in translate.go does not process these new fields.

Resource.Validate in validate.go does not include them in the check for mutually exclusive source fields (e.g., inline, local, source). This could allow users to specify multiple sources, leading to ambiguity.
Is the implementation for these fields planned for a future pull request? If so, it might be better to introduce the schema changes and their implementation in the same PR to avoid having unused fields in the schema.

gemini-code-assist · 2025-08-02T11:51:40Z

config/common/translate.go

+// translators take a raw config and translate it to a raw Ignition config. The report returned should include any
+// errors, warnings, etc. and may or may not be fatal. If report is fatal, or other errors are encountered while translating
+// translators should return an error.
+type translator func([]byte, TranslateBytesOptions) ([]byte, report.Report, error)


The translator type is used in the exported function RegisterTranslator. While Go allows using unexported types in exported function signatures if they are function types, it's more idiomatic and better for API clarity to export the type. This makes it easier for users of the package to understand and implement the required interface.
Consider renaming translator to Translator to make it public. You will also need to update its usage in this file (e.g., in registry, RegisterTranslator, and getTranslator).

Suggested change

type translator func([]byte, TranslateBytesOptions) ([]byte, report.Report, error)

type Translator func([]byte, TranslateBytesOptions) ([]byte, report.Report, error)

vic1707 · 2025-08-03T17:57:20Z

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 vic1707/homelab-config#52)

But the inheritance of the filesDir can quickly become a pain, I'll reread #118 and implement one of the idea (separate branch in case you don't want both at the same time)

vic1707 · 2025-08-04T18:32:48Z

Added the possibility to use a new custom files-dir for imported butanes: vic1707#4 (only a local PR for now if the solution proposed is accepted)

prestist · 2025-08-14T15:13:46Z

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.

prestist

Sorry for the delay. I want to play around with the changes and get a better understanding but here is my first pass.

Thank you for working on this!

prestist · 2025-08-21T14:59:36Z

docs/config-fcos-v1_7-exp.md

        * **_source_** (string): the URL of the certificate bundle (in PEM format). The bundle can contain multiple concatenated certificates. Supported schemes are `http`, `https`, `tftp`, `s3`, `arn`, `gs`, and [`data`](https://tools.ietf.org/html/rfc2397). When using `http`, it is advisable to use the verification option to ensure the contents haven't been modified. Mutually exclusive with `inline` and `local`.
        * **_inline_** (string): the contents of the certificate bundle (in PEM format). The bundle can contain multiple concatenated certificates. Mutually exclusive with `source` and `local`.
        * **_local_** (string): a local path to the contents of the certificate bundle (in PEM format), relative to the directory specified by the `--files-dir` command-line argument. The bundle can contain multiple concatenated certificates. Mutually exclusive with `source` and `inline`.
+        * **_inline_butane_** (string): the contents of the certificate bundle. Mutually exclusive with `source` and `local`.


Hmm, this feels odd; probably coming from the fact that the new fields are in "resource". (noting for self need to check about this)

Maybe I can divide the Resource type?
One for "classic" ressources, another one for merged configs?

prestist · 2025-08-27T12:39:33Z

base/v0_7_exp/tests/validate_test.go

 // limitations under the License.)

-package v0_7_exp
+package tests


Sorry, what cycle issue were you hitting?

The issue arises because I wanted to use cfg.TranslateBytes() (config/config.go:128).

Importing cfg "github.com/coreos/butane/config" in base/v0_7_exp/translate.go causes the import cycle not allowed error as the config module imports every config translator (ie: config/fcos/v1_7_exp) which internally import their own base schemas (ie: base/v0_7_exp) which causes the cyclic import issue.

I tried my best to fix this but the init function of the config modules beat me 😓.
I found 2 solutions when trying to fix it without changing too much existing code:

the one you're currently testing/reviewing:

creating a submodule translate in the config module so imports are fine

abusing the fact that every entrypoint in butane imports the config module somewhere (mainly in each main.go) to load the init function

(can be seen here alt solution? vic1707/butane#3)

Using the reflexion system to avoid importing anything (but as I don't know go I don't really know if I did it right, it just works as is 😓)

In both cases I'm not a fan of the solutions I came up with as I'm convinced a more experienced go developer can do better (help, hints appreciated!)

prestist · 2025-08-27T12:50:25Z

base/v0_7_exp/translate.go

-	tr.AddCustomTranslator(translatePasswdUser)
-	tr.AddCustomTranslator(translateUnit)
+	tr.AddCustomTranslator(TranslateIgnition)
+	tr.AddCustomTranslator(TranslateFile)


lets keep these lower case, in go uppercase makes the func public.

You're right, but without figuring out a new fix for the circular dependency it has to be exported (the tests from the new tests module import them).
Circular issue explained in another comment

prestist · 2025-08-27T12:51:37Z

base/v0_7_exp/translate.go

 			}
 		}
-
-		src, compression, err := baseutil.MakeDataURL(contents, to.Compression, !options.NoResourceAutoCompression)


Hmm, if we want to simplify existing code it might be better to do that in a follow up pr to avoid confusion.

no prob, once we have an idea of all the things I have to change I'll make 2 distincts PRs 👍

prestist · 2025-08-27T12:58:04Z

config/common/translate.go

+	registry = map[string]translator{}
+)
+
+// Fields that must be included in the root struct of every spec version.


Hmm, why did you want to make a "translate.go" in common. If you want to effect all targets I think we would put it in https://github.com/coreos/butane/blob/main/base/v0_7_exp/translate.go

this is due to the circular imports, I don't want to do that, I had to 😓
As for moving this stuff to base/v0_7_exp/translate.go I'll take a look, this will probably be better 👍

vic1707 · 2025-08-28T12:05:07Z

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 vic1707#4 which allows for an overwrite of the files-dir for imported butane configs (it really convenient when managing multiple root butane configs)

janus-reith · 2025-11-05T18:19:09Z

This is a great addition, I hope it won't go stale

vic1707 · 2025-11-05T19:12:25Z

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 !

prestist · 2025-11-13T20:04:30Z

@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.

prestist · 2025-11-13T21:36:37Z

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

vic1707 · 2025-11-14T17:42:33Z

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)

prestist · 2025-11-14T19:47:50Z

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.

vic1707 · 2025-11-14T21:21:44Z

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.

vic1707 changed the title ~~Butane merge~~ [WIP] Butane merge Aug 2, 2025

gemini-code-assist bot reviewed Aug 2, 2025

View reviewed changes

vic1707 force-pushed the butane-merge branch 6 times, most recently from cc151df to 12d513e Compare August 2, 2025 17:23

vic1707 added 3 commits August 2, 2025 19:30

Add new local_butane and inline_butane props

882b4fa

move global translate function to avoid cycle imports

df0f56e

extract ignition treatment

7829756

vic1707 force-pushed the butane-merge branch 4 times, most recently from 7d029aa to 766eaa9 Compare August 2, 2025 20:26

vic1707 marked this pull request as draft August 3, 2025 12:56

vic1707 added 4 commits August 3, 2025 17:49

handle both new merge cases

299819b

add the missing line, still cycle imports

b2c41ca

solution for the import cycle

4021d10

simplify TranslateResource

56b588a

vic1707 force-pushed the butane-merge branch from 766eaa9 to 56b588a Compare August 3, 2025 15:49

vic1707 added 3 commits August 3, 2025 17:54

edit release notes

c8dbe65

missing validation logic for inline_butane

ef1a297

dedup translateButaneRessource

1c19030

vic1707 marked this pull request as ready for review August 3, 2025 16:13

vic1707 changed the title ~~[WIP] Butane merge~~ Butane merge Aug 3, 2025

prestist reviewed Aug 27, 2025

View reviewed changes

vic1707 mentioned this pull request Nov 14, 2025

Tree permissions #644

Merged

vic1707 mentioned this pull request Nov 16, 2025

chore: package dependencies debt #656

Draft

		InlineButane *string `yaml:"inline_butane"`
		LocalButane *string `yaml:"local_butane"`

	type translator func([]byte, TranslateBytesOptions) ([]byte, report.Report, error)
	type Translator func([]byte, TranslateBytesOptions) ([]byte, report.Report, error)

Butane merge #631

Are you sure you want to change the base?

Butane merge #631

Uh oh!

Conversation

vic1707 commented Aug 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Aug 2, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Aug 2, 2025

Choose a reason for hiding this comment

Uh oh!

vic1707 commented Aug 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vic1707 commented Aug 4, 2025

Uh oh!

prestist commented Aug 14, 2025

Uh oh!

prestist left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vic1707 Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vic1707 Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vic1707 commented Aug 28, 2025

Uh oh!

janus-reith commented Nov 5, 2025

Uh oh!

vic1707 commented Nov 5, 2025

Uh oh!

prestist commented Nov 13, 2025

Uh oh!

prestist commented Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The cycle issue:

Uh oh!

vic1707 commented Nov 14, 2025

Uh oh!

prestist commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vic1707 commented Nov 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vic1707 commented Aug 2, 2025 •

edited

Loading

vic1707 commented Aug 3, 2025 •

edited

Loading

vic1707 Aug 28, 2025 •

edited

Loading

vic1707 Aug 28, 2025 •

edited

Loading

prestist commented Nov 13, 2025 •

edited

Loading

prestist commented Nov 14, 2025 •

edited

Loading