Skip to content

internal: use seq iterators to avoid allocations - #2354

Merged
angelcerveraroldan merged 1 commit into
coreos:mainfrom
cadejacobson:splitseq
Oct 30, 2026
Merged

angelcerveraroldan merged 1 commit into
coreos:mainfrom
cadejacobson:splitseq

Conversation

@coderabbitai

coderabbitai Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 1f39eb3 and 14891cb.

Files selected for processing: 7
  • docs/release-notes.d/string-iterators.change.md
  • internal/exec/stages/disks/luks.go
  • internal/exec/stages/disks/partitions.go
  • internal/providers/cmdline/cmdline.go
  • internal/providers/vmware/ovf_test.go
  • internal/providers/zvm/zvm.go
  • tests/positive/general/preemption.go

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

Recent review details
Context from checks skipped due to timeout: 4
  • GitHub Check: CodeRabbit
  • GitHub Check: Test (1.26.x)
  • GitHub Check: Test (1.25.x)
  • GitHub Check: tmt-tests
Additional context used
Code guidelines: 1
AGENTS.md — auto-discovered
Path-based instructions: 5
Execution stages are fixed (fetch-offline, fetch, disks, mount, files, umount).

CodeRabbit configuration file

Files:

  • internal/exec/stages/disks/luks.go
  • internal/exec/stages/disks/partitions.go
Platform providers must retry config fetch, allow empty config, and never guess the platform ID.

CodeRabbit configuration file

Files:

  • internal/providers/zvm/zvm.go
  • internal/providers/cmdline/cmdline.go
  • internal/providers/vmware/ovf_test.go
Blackbox integration tests.

CodeRabbit configuration file

Files:

  • tests/positive/general/preemption.go
Documentation served via GitHub Pages/Jekyll.

CodeRabbit configuration file

Files:

  • docs/release-notes.d/string-iterators.change.md
Source excerpt: **License header**: Required on all `.go` files (Apache 2.0, 13-line header)

CodeRabbit inference engine (AGENTS.md)

Files:

  • internal/exec/stages/disks/luks.go
  • internal/providers/zvm/zvm.go
  • internal/providers/cmdline/cmdline.go
  • internal/exec/stages/disks/partitions.go
  • tests/positive/general/preemption.go
  • internal/providers/vmware/ovf_test.go
Learnings: 1
Learning: 2026-10-20T08:26:09.855Z
Learnt from: CR
Repo: coreos/ignition PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-10-20T08:26:09.855Z
Learning: Source excerpt:
# Ignition

## Code Style

- **Formatting**: `gofmt` enforced (CI and `./test`)

Applied to files:

  • internal/exec/stages/disks/luks.go
Additional comments: 7
internal/providers/cmdline/cmdline.go: 1

108-108: LGTM!

internal/exec/stages/disks/luks.go: 1

625-625: LGTM!

internal/exec/stages/disks/partitions.go: 1

255-256: LGTM!

internal/providers/zvm/zvm.go: 1

67-67: LGTM!

tests/positive/general/preemption.go: 1

60-60: LGTM!

internal/providers/vmware/ovf_test.go: 1

160-160: LGTM!

docs/release-notes.d/string-iterators.change.md: 1

1-1: LGTM!

Walkthrough

Several parsing and test loops now use string iterators instead of allocating slices from split strings or fields. A release-note fragment records the change. The parsing and processing logic remains unchanged.

Changes: String iterator adoption

Layer / File(s) Summary
Replace slice-based string iteration
internal/providers/cmdline/cmdline.go, internal/exec/stages/disks/luks.go, internal/exec/stages/disks/partitions.go, internal/providers/zvm/zvm.go, tests/positive/general/preemption.go, internal/providers/vmware/ovf_test.go, docs/release-notes.d/string-iterators.change.md
Loops use strings.FieldsSeq or strings.SplitSeq instead of iterating over slices created by strings.Fields or strings.Split. The release note describes the iterator change.

Priority: Low

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

Change: Refactor

Possibly related issues

  • Issue 2353 describes the release-note fragment workflow used by this change.

Possibly related PRs

Suggested reviewers: bgilbert

Merge Risk: Minimal · up to 14891

This allocation-focused change is ready to merge; no material current-head risk was identified.

Pre-merge checks: Passed 5 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 5 passed
Check name Status Explanation
Title check Passed The title follows the required format: it uses the subsystem prefix "internal", a lowercase imperative description, and no trailing period. It accurately summarizes the iterator allocation changes.
Description check Passed The description directly explains the changes from strings.Split and strings.Fields to strings.SplitSeq and strings.FieldsSeq, including the allocation benefit and supporting references.
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 PR has one non-merge commit: internal: use string iterators to avoid allocations. internal is a valid subsystem path prefix, use starts the description in lowercase imperative mood, and the …
Full details: Commit Message Convention

Explanation

The PR has one non-merge commit: internal: use string iterators to avoid allocations. internal is a valid subsystem path prefix, use starts the description in lowercase imperative mood, and the message has no trailing period.

Full details: Docstring Coverage

Explanation

Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 6 functions across 6 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@github-actions

Copy link
Copy Markdown

Binary size report (bin/amd64/ignition)

Size
Base (main) 33MiB
PR (#2354) 33MiB
Delta +5.6KiB (0.02%)

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

I did not know that these methods existed, neat.

From the docs:

The iterator yields the same strings that would be returned by Fields(s), but without constructing the slice.

The iterator yields the same strings that would be returned by Split(s, sep), but without constructing the slice. It returns a single-use iterator

So it LGTM, thanks.

@angelcerveraroldan
angelcerveraroldan merged commit 15e68a3 into coreos:main Oct 30, 2026
14 checks passed
@cadejacobson
cadejacobson deleted the splitseq branch November 2, 2026 13:13
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.

2 participants