Correct partition index loop in translate.go by skewballfox · Pull Request #722 · coreos/butane

skewballfox · 2026-07-09T02:43:39Z

currently trying to override the default sizes for the boot partitions.

In my butain file, I have:

        - number: 1
          label: bios
          size_mib: 1
          type_guid: 21686148-6449-6E6F-744E-656564454649
        - number: 2
          label: efi 
          size_mib: 1024
          type_guid: C12A7328-F81F-11D2-BA4B-00A0C93EC93B
          wipe_partition_entry: true
        - number: 3
          label: boot
          size_mib: 2048
          type_guid: 0FC63DAF-8483-4772-8E79-3D69D8477DE4
          wipe_partition_entry: true
        - label: root
          number: 4
          wipe_partition_entry: true
          type_guid: 4F68BCE3-E8CD-4DB1-96E7-FBCAF984B709

which should be valid. root is the last partition, and all the others have a set size. currently this throws:

     |                  ^^^^ root partition cannot expand; it is set to fill available space but is followed by an auto-positioned partition
 122 |           number: 4
 123 |           start_mib: 0
     |
Config produced warnings and --strict was specified

this shoud only check the partitions after root

Summary by CodeRabbit

Bug Fixes
- Refined root-partition constraint validation during configuration conversion by limiting checks to partitions after the root entry, preventing incorrect or overly broad warnings for valid disk layouts.
- Added/extended test coverage to confirm that when root is the last partition (with preceding partitions explicitly sized), no root constraint warning is emitted.

Fix loop to start from the next partition index.

coderabbitai · 2026-07-09T02:43:56Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Enterprise

Run ID: 5154623e-6970-4f2d-82db-c4dd107fc6b5

📥 Commits

Reviewing files that changed from the base of the PR and between 8815ea6 and 6f1cfcb.

📒 Files selected for processing (11)

config/fcos/v1_3/translate.go
config/fcos/v1_3/translate_test.go
config/fcos/v1_4/translate.go
config/fcos/v1_4/translate_test.go
config/fcos/v1_5/translate_test.go
config/fcos/v1_6/translate.go
config/fcos/v1_6/translate_test.go
config/fcos/v1_7/translate.go
config/fcos/v1_7/translate_test.go
config/fcos/v1_8_exp/translate.go
config/fcos/v1_8_exp/translate_test.go

📝 Walkthrough

Walkthrough

Root partition constraint checks across FCOS v1.3 through v1.8 experimental translations now inspect only partitions after the root partition. Regression tests cover final root partitions with explicitly sized preceding partitions and expect no warning.

Changes

Root partition constraint validation

Layer / File(s)	Summary
Narrow root constraint partition scans `config/fcos/v1_3/translate.go`, `config/fcos/v1_4/translate.go`, `config/fcos/v1_5/translate.go`, `config/fcos/v1_6/translate.go`, `config/fcos/v1_7/translate.go`, `config/fcos/v1_8_exp/translate.go`	Root constraint loops now start at `p + 1`, excluding partitions before the root partition.
Final root partition regression coverage `config/fcos/v1_3/translate_test.go`, `config/fcos/v1_4/translate_test.go`, `config/fcos/v1_5/translate_test.go`, `config/fcos/v1_6/translate_test.go`, `config/fcos/v1_7/translate_test.go`, `config/fcos/v1_8_exp/translate_test.go`	Added table-driven cases where sized partitions precede a final root partition and the expected report is empty.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title is concise and clearly matches the main fix: adjusting the partition index loop in translate.go.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

_{Comment @coderabbitai help to get the list of available commands.}

gemini-code-assist

Code Review

This pull request optimizes the partition iteration logic in config/fcos/v1_5/translate.go by starting the loop from p + 1 instead of iterating over all partitions. However, this optimization makes the inner check idx == p redundant, and it should be removed to clean up the code.

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.

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

skewballfox · 2026-07-10T15:17:08Z

btw, I tried compiling locally and testing out this with the butane file containing the code above, for some reason even though this should start from the partitions after root (if any), it seems the same error message, so I'm guessing either the array of partitions isn't in the declared order, or some partition is part of that array that isn't one the ones defined.

prestist · 2026-07-17T19:58:05Z

Thanks for the fix @skewballfox! I agree this is a bug; the loop should only check partitions declared after root, since sgdisk processes them in declaration order and only subsequent auto-positioned partitions would constrain root's expansion. This won't break anyone since it only removes a false warning.
A couple things needed before we can merge:

Apply the fix to all spec versions
The same bug exists in v1_3, v1_4, v1_6, v1_7, and v1_8_exp (config/fcos/*/translate.go). Since this is a bug fix and won't change behavior for configs that are currently valid, it should be safe to fix everywhere.
Add a test case
Could you add a test case to TestRootPartitionConstraints where root is the last partition with sized partitions before it (matching your original scenario)? This would prevent the regression. Something like:

{
	name: "root last with sized partitions before",
	in: Config{
		Config: base.Config{
			Storage: base.Storage{
				Disks: []base.Disk{
					{
						Device: "/dev/vda",
						Partitions: []base.Partition{
							{
								Label:   util.StrToPtr("bios"),
								Number:  1,
								SizeMiB: util.IntToPtr(1),
							},
							{
								Label:   util.StrToPtr("boot"),
								Number:  3,
								SizeMiB: util.IntToPtr(2048),
							},
							{
								Label:  util.StrToPtr("root"),
								Number: 4,
							},
						},
					},
				},
			},
		},
	},
	report: report.Report{}, // no warning expected
},

Regarding the warning still appearing
You mentioned the fix didn't resolve the warning when you tested. Since this PR only patches v1_5, if your Butane config uses a different version (e.g., version: "1.6.0"), it would hit the unfixed code. Fixing all versions (point 1) should resolve that.
Commit conventions
The project uses the format component: description -- for this change: */translate: fix root partition constraint check to only examine subsequent partitions

skewballfox · 2026-07-17T20:43:38Z

sorry, been afk a bit. I'll push the changes today or tomorrow

…sequent partitions

Correct partition index loop in translate.go

367706a

Fix loop to start from the next partition index.

gemini-code-assist Bot reviewed Jul 9, 2026

View reviewed changes

Comment thread config/fcos/v1_5/translate.go Outdated

Update config/fcos/v1_5/translate.go

8815ea6

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

*/translate: fix root partition constraint check to only examine sub…

6f1cfcb

…sequent partitions

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Correct partition index loop in translate.go#722

Correct partition index loop in translate.go#722
skewballfox wants to merge 3 commits into
coreos:mainfrom
skewballfox:patch-2

skewballfox commented Jul 9, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jul 9, 2026 •

edited

Loading

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

Uh oh!

skewballfox commented Jul 10, 2026

Uh oh!

prestist commented Jul 17, 2026

Uh oh!

skewballfox commented Jul 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

skewballfox commented Jul 9, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

skewballfox commented Jul 10, 2026

Uh oh!

prestist commented Jul 17, 2026

Uh oh!

skewballfox commented Jul 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

skewballfox commented Jul 9, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 9, 2026 •

edited

Loading