Skip to content

butane: import as git subtree (alternative to #2235)#2237

Closed
prestist wants to merge 1301 commits into
coreos:mainfrom
prestist:butane-subtree-merge
Closed

butane: import as git subtree (alternative to #2235)#2237
prestist wants to merge 1301 commits into
coreos:mainfrom
prestist:butane-subtree-merge

Conversation

@prestist

@prestist prestist commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

This is an alternative to #2235 that uses a git subtree merge to bring butane into ignition.
The main difference: butane stays a separate Go module with its own go.mod, and the coreos/butane
repo can keep functioning independently.

Syncing future butane changes is straightforward:

git fetch butane-remote main
git merge -s subtree --allow-unrelated-histories butane-remote/main
#2235 (direct copy) this PR (subtree)
git history butane commits inlined into PR preserved as merge parent
Go module single module, imports rewritten separate modules, butane unchanged
butane repo archived stays functional
upstream sync manual git merge -s subtree
build one go build sub-build via butane/build

This is a proof-of-concept -- it doesn't include the transpilation integration,
vendor changes, or import rewriting that #2235 has. Those would be follow-up work.

yasminvalim and others added 30 commits June 5, 2024 15:39
Stabilize on ignition 3.4, fcos 1.5 and base 0.5 specs
…able

openshift/v4.16: Stabilize 4.16.0 spec and add openshift 4.17-experimental spec
docs/release-notes.md: update for release Butane 0.21.0
Dockerfile: update to Fedora 40
Step down to non expermental features

Additionaly remove GRUB password support
See: https://issues.redhat.com/browse/MCO-630
- Use "layout" in the context path for errors related to the layout
  entry in the boot_device  configuration.
- Add tests for those error cases.
- Refactor mirror boot_device check for s390x.

Fixes: coreos/butane#484
For fcos 1.6.0-exp & openshift 4.18.0-exp specs, expected to be based on
stable 3.5.0 spec.

See: coreos#1693
See: coreos/fedora-coreos-tracker#1708
add: Support LUKS encryption using IBM CEX secure keys on s390x
prestist and others added 23 commits May 19, 2026 11:21
- Regenerate spec documentation
- Update docs/specs.md to move 4.22.0 to stable and add 4.23.0-experimental
- Update docs/upgrading-openshift.md for 4.21.0 to 4.22.0 migration
- Update docs/release-notes.md to note stabilization and remove -exp suffixes
docs/release-notes: update for release v0.28.0
docs/release-notes: fix openshift quadlets version reference
Dockerfile: update to Fedora 44
docs/release-notes: note Fedora 44 signing key
Config.Validate() warns when a partition on a non-boot disk uses a
label without an explicit number and wipe_table is not set.  However,
when the disk is listed in boot_device.mirror.devices, this warning is
a false positive: processBootDevice() in translate.go will auto-
generate wipe_table: true for mirror disks, but Validate() runs
before translation so it doesn't see the auto-generated wipe_table.

The user can't reasonably suppress this: adding explicit wipe_table:
true is redundant, and adding explicit partition number fields would
break the label-based merge with the auto-generated mirror partitions.

Skip the ErrReuseByLabel check for disks that are listed in
boot_device.mirror.devices.

Fixes: coreos/butane#710

Assisted-by: <anthropic/claude-opus-4.6>
…boot-mirror

fcos: skip ErrReuseByLabel warning for boot_device.mirror disks
Unit tests that use pre-computed compressed data will fail in GoTip as
the output of the compression function has changed.

More importantly, the output is not guaranteed to be stable, so tests
should now generate the zipped data at test-time, and we pre-define
the expected input string.
The exact byte output of compressing functions may not be stable
across different versions, so hard coding the expected byte output of
the compression function will make tests fail or pass depending on the
version of Go used to run the test.
Add an optional flag to to enable pretty rust-like error reporting for
butane errors.

This introduces the flags:
- raw-error
- color / colour	(auto, always, or never)

While also respecting the `NO_COLOR` environment variable.
…-tests

*/translate_test: Do not pre-compute compressed data
Import the coreos/butane repository under the butane/ directory using
the git subtree merge strategy. This preserves butane's full commit
history as a parent of this merge commit, while placing all files
under the butane/ prefix.

Future upstream changes can be pulled with:
  git fetch butane-github main
  git merge -s subtree --allow-unrelated-histories butane-github/main

This approach keeps the coreos/butane repo independently functional
and allows bidirectional synchronization.
Update the build script to invoke butane's own build script from the
subtree directory, preserving butane's independent Go module. Update
the Makefile to install the butane binary alongside ignition-validate.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request integrates the butane project as a subtree, updating the top-level build scripts and Makefile to compile and install it, alongside adding its core specs, utilities, tests, and automation workflows. The code review identified several issues: a relative path bug in the build script, a logical error in translateResource that bypasses local Ignition config validation, potential edge cases in path traversal checks, variable shadowing of the report package, inefficient regex compilation in a loop, and a reflection typo in map key validation within tests.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread build

# Build butane from the subtree (separate Go module)
echo "Building butane..."
(cd butane && BIN_PATH=${BIN_PATH} ./build)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

If BIN_PATH is a relative path (e.g., bin/amd64), changing the directory to butane will cause the sub-build to write the binary to butane/bin/amd64/butane instead of the top-level bin/amd64/butane. This will cause the top-level Makefile installation to fail because it expects the binary at the top-level directory.

To fix this, resolve BIN_PATH to an absolute path before changing the directory.

abs_bin_path=$(mkdir -p "${BIN_PATH}" && cd "${BIN_PATH}" && pwd)
(cd butane && BIN_PATH="${abs_bin_path}" ./build)

Comment on lines +128 to +134
if strings.HasPrefix(c.String(), "$.ignition.config") {
rp, err := ValidateIgnitionConfig(c, contents)
r.Merge(rp)
if err != nil {
return
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The check strings.HasPrefix(c.String(), "$.ignition.config") is a logical error and will always evaluate to false. This is because c is defined locally on line 120 as c := path.New("yaml", "local"), which has a string representation of "yaml.local" (not starting with "$.ignition.config"). As a result, local Ignition configs specified via local are never validated.

To fix this, split translateResource into two translators:

  1. translateResource (for general resources, without Ignition config validation).
  2. translateIgnitionResource (specifically for Ignition configs, which performs the validation).

Then, register translateIgnitionResource in translateIgnition, and translateResource in translateFile.

Comment thread butane/base/util/file.go
Comment on lines +34 to +36
if absPath != absBase && !strings.HasPrefix(absPath, absBase+string(filepath.Separator)) {
return common.ErrFilesDirEscape
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using manual string concatenation and strings.HasPrefix to check if a path is within a base directory can lead to edge cases, such as when filesDir is the root directory / (where absBase + Separator becomes //, causing false positives). A more robust and idiomatic approach in Go is to use filepath.Rel and check if the relative path starts with ...

Suggested change
if absPath != absBase && !strings.HasPrefix(absPath, absBase+string(filepath.Separator)) {
return common.ErrFilesDirEscape
}
rel, err := filepath.Rel(absBase, absPath)
if err != nil {
return err
}
if strings.HasPrefix(rel, "..") {
return common.ErrFilesDirEscape
}

Comment thread butane/base/v0_2/util.go
Comment on lines +154 to +155
report := validate.ValidateWithContext(config, rawConfig)
r.Merge(report)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The variable name report shadows the imported package name github.com/coreos/vcontext/report. It is better to rename it to valReport or vr to avoid shadowing and improve code clarity.

Suggested change
report := validate.ValidateWithContext(config, rawConfig)
r.Merge(report)
valReport := validate.ValidateWithContext(config, rawConfig)
r.Merge(valReport)

r.AddOnError(c.Append(keyFileIndex), err)
continue
}
for _, line := range regexp.MustCompile("\r?\n").Split(string(sshKeys), -1) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Compiling a regular expression inside a loop or function that is called repeatedly is inefficient. Since translatePasswdUser can be called multiple times (once for each user), compiling the regex on every invocation is a performance bottleneck.

You can avoid using a regular expression entirely by using strings.Split and strings.ReplaceAll to handle both Unix and Windows line endings, which is significantly faster and more memory-efficient.

Suggested change
for _, line := range regexp.MustCompile("\r?\n").Split(string(sshKeys), -1) {
for _, line := range strings.Split(strings.ReplaceAll(string(sshKeys), "\r\n", "\n"), "\n") {

Comment thread butane/base/util/test.go
switch v.Kind() {
case reflect.Map:
value := v.MapIndex(reflect.ValueOf(p.Path[0]))
if v.IsZero() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The check if v.IsZero() is a typo and should be if !value.IsValid(). Currently, it checks the map v itself instead of the retrieved element value. If a key does not exist in the map, v.MapIndex returns an invalid reflect.Value, which bypasses this check and eventually falls through to the default case with an incorrect error message.

Suggested change
if v.IsZero() {
if !value.IsValid() {

@prestist prestist closed this Jun 18, 2026
@prestist prestist reopened this Jun 18, 2026
@prestist prestist changed the title butane: add butane as git subtree merge (alternative to #2235) butane: import as git subtree (alternative to #2235) Jun 18, 2026
@prestist prestist marked this pull request as draft June 18, 2026 20:12
@prestist prestist closed this Jun 23, 2026
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.