Skip to content

Correct partition index loop in butane/*/translate.go - #2304

Merged
angelcerveraroldan merged 1 commit into
coreos:mainfrom
skewballfox:translate-fix
Sep 21, 2026
Merged

angelcerveraroldan merged 1 commit into
coreos:mainfrom
skewballfox:translate-fix

Conversation

@skewballfox

Copy link
Copy Markdown
Contributor

patching work from PR 722 for butane.

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

@coderabbitai

coderabbitai Bot commented Aug 24, 2026 •

Copy link
Copy Markdown

Review Change StackReview Change Stack

Understand this PR’s impact

Explore downstream dependencies and potential security impact with Blast Radius.

View blast radius →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: coreos/ignition/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: f95ad5cc-cad5-4096-aa61-7167e3ce6364

📥 Commits

Reviewing files that changed from the base of the PR and between 17a2025 and 323df1d.

📒 Files selected for processing (1)
  • docs/release-notes.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • docs/release-notes.md

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.


📝 Walkthrough

Walkthrough

Root partition constraint checks now inspect only partitions after the root partition across FCOS versions 1.3 through 1.8 experimental. Regression tests cover final unsized root partitions preceded by explicitly sized partitions. Release notes document the fix.

Changes

Root partition constraints

Layer / File(s) Summary
Constraint validation and regression coverage
butane/config/fcos/v1_3/..., butane/config/fcos/v1_4/..., butane/config/fcos/v1_5/..., butane/config/fcos/v1_6/..., butane/config/fcos/v1_7/..., butane/config/fcos/v1_8_exp/..., docs/release-notes.md
Root partition validation now checks only subsequent partitions. Tests verify that preceding sized partitions do not trigger a warning for a final unsized root partition. The release notes describe the fix.

Priority: ⬇️ Low

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

Change: Bug fix

Suggested reviewers: prestist

🚥 Pre-merge checks | ✅ 5 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Title check ⚠️ Warning The title describes the partition index loop change, but it does not follow the required format. It lacks a subsystem prefix and uses uppercase text after the start instead of a lowercase description. Rewrite the title as subsystem: lowercase imperative description, such as config: correct partition index loop. Do not add a trailing period.
✅ Passed checks (5 passed)
Check name Status Explanation
Description check ✅ Passed The description explains the root partition warning, provides a representative configuration, and identifies the intended fix. It is directly related to the changeset.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
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.
Commit Message Convention ✅ Passed The reviewed range contains one non-merge commit. Its subject is butane/ */translate: fix root partition constraint check to only examine subsequent partitions. The subject has a subsystem, a colon-…
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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

@github-actions

github-actions Bot commented Aug 24, 2026 •

Copy link
Copy Markdown

Binary size report (bin/amd64/ignition)

Size
Base (main) 33MiB
PR (#2304) 33MiB
Delta -8B (-0.00%)

@angelcerveraroldan angelcerveraroldan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the PR!

I tested the changes locally, and the bug is fixed correctly. I did leave one small nit about the edit to butane/docs/release-notes.md.

Comment thread butane/docs/release-notes.md Resolved

@prestist prestist left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I rebased, it and fixed the nit! thank you for your work on this @skewballfox !

@angelcerveraroldan angelcerveraroldan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@angelcerveraroldan
angelcerveraroldan merged commit f68687f into coreos:main Sep 21, 2026
13 checks passed
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.

3 participants