Skip to content

fix: add oci:// prefix to default flash command and preserve flashCmd#226

Merged
bennyz merged 1 commit into
centos-automotive-suite:mainfrom
bennyz:worktree-fix-flash-oci-prefix
Apr 16, 2026
Merged

fix: add oci:// prefix to default flash command and preserve flashCmd#226
bennyz merged 1 commit into
centos-automotive-suite:mainfrom
bennyz:worktree-fix-flash-oci-prefix

Conversation

@bennyz

@bennyz bennyz commented Apr 15, 2026

Copy link
Copy Markdown
Contributor

The default flash command was missing the oci:// scheme prefix

Additionally, passing --exporter to override the exporter selector was skipping the entire target mapping lookup, silently dropping the configured flashCmd (and its flags like --fls-version, --no-power-off). Now --exporter only overrides the selector while still inheriting flashCmd from the target mapping.

Summary

Related Issues

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update
  • CI/CD improvement
  • Refactoring

Testing

  • Unit tests pass (make test)
  • Linter passes (make lint)
  • Manifests are up to date (make manifests generate)
  • Tested on OpenShift cluster (if applicable)

Summary by CodeRabbit

  • Improvements
    • Flash operations now default to using the oci:// URI scheme for image references when no custom flash command is provided, ensuring consistent image addressing.
    • Target mapping resolution for flash tasks has been made more reliable: mappings that provide flash commands are consulted more broadly, and explicit exporter selector values are preserved unless absent, reducing unexpected overrides.

@coderabbitai

coderabbitai Bot commented Apr 15, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 988b97f0-fb69-4a70-a292-9a8c8247835b

📥 Commits

Reviewing files that changed from the base of the PR and between 89e4955 and f08b64a.

📒 Files selected for processing (4)
  • internal/buildapi/flash_helpers.go
  • internal/common/tasks/scripts/flash_image.sh
  • internal/common/tasks/tasks.go
  • internal/controller/imagebuild/controller.go
✅ Files skipped from review due to trivial changes (2)
  • internal/common/tasks/scripts/flash_image.sh
  • internal/common/tasks/tasks.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • internal/controller/imagebuild/controller.go

📝 Walkthrough

Walkthrough

Default flash command template now prepends the oci:// scheme when not user-supplied; controller and buildapi mapping lookup logic now always consults Jumpstarter target mappings for flashCmd when a Target is provided, while selector overrides are only applied if not already set.

Changes

Cohort / File(s) Summary
Flash script
internal/common/tasks/scripts/flash_image.sh
Default FLASH_CMD template changed from j storage flash {image_uri} to j storage flash oci://{image_uri}; user-supplied FLASH_CMD unchanged.
Tekton param docs
internal/common/tasks/tasks.go
Updated flash-cmd ParamSpec description examples to show oci://{image_uri} instead of bare {image_uri}.
Controller & buildapi flash resolution
internal/controller/imagebuild/controller.go, internal/buildapi/flash_helpers.go
Mapping lookup now occurs whenever Target is set and Jumpstarter config exists; mapping.FlashCmd is always used when present, and exporterSelector/flashExporterSelector are only overridden from mapping when the request/override is empty. Missing-mapping error check moved after lookup.

Sequence Diagram(s)

(Skipped — changes are localized control-flow adjustments without a new multi-component sequential feature.)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • bkhizgiy

Poem

🐰 I nibble code with eager paws,
oci:// now leads the cause.
Mappings checked where targets hop,
Selectors stay unless they stop,
The rabbit grins — the flashes pop ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main changes: adding the oci:// prefix to the default flash command and preserving flashCmd when exporter is overridden.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@internal/common/tasks/scripts/flash_image.sh`:
- Around line 22-23: The default FLASH_CMD uses an escaped placeholder
"\{image_uri\}" so the subsequent sed substitution FLASH_CMD=$(echo
"${FLASH_CMD}" | sed "s|{image_uri}|${IMAGE_REF}|g") doesn't match; update the
default assignment of FLASH_CMD to use unescaped braces "{image_uri}" (i.e.,
FLASH_CMD="${FLASH_CMD:-j storage flash oci://{image_uri}}") so the sed
replacement against IMAGE_REF correctly replaces the placeholder.

In `@internal/common/tasks/tasks.go`:
- Line 847: Update the flash-cmd description strings to use the supported
placeholder {image_uri} instead of the unsupported ${IMAGE_REF}; locate the
Description fields that mention "Custom flash command" (e.g., the Description
entry in the task definitions around the flash-cmd option in
internal/common/tasks/tasks.go and the similar occurrence later) and replace any
`${IMAGE_REF}` examples with `{image_uri}` so the help text matches the runtime
placeholder the script rewrites before calling the exporter.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: c0132a63-247c-4758-bc1a-d52d21ab058c

📥 Commits

Reviewing files that changed from the base of the PR and between 9e9c5f1 and 2cdb0cd.

📒 Files selected for processing (3)
  • internal/common/tasks/scripts/flash_image.sh
  • internal/common/tasks/tasks.go
  • internal/controller/imagebuild/controller.go

Comment thread internal/common/tasks/scripts/flash_image.sh Outdated
Comment thread internal/common/tasks/tasks.go Outdated
@bennyz bennyz force-pushed the worktree-fix-flash-oci-prefix branch from 2cdb0cd to 89e4955 Compare April 15, 2026 16:48
@bennyz bennyz requested a review from bkhizgiy April 15, 2026 17:12
The default flash command was missing the oci:// scheme prefix

Additionally, passing --exporter to override the exporter selector was
skipping the entire target mapping lookup, silently dropping the
configured flashCmd (and its flags like --fls-version, --no-power-off).
Now --exporter only overrides the selector while still inheriting
flashCmd from the target mapping.

Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Assisted-by: claude-sonnet-4.6
@bennyz bennyz force-pushed the worktree-fix-flash-oci-prefix branch from 89e4955 to f08b64a Compare April 16, 2026 15:52
@bennyz bennyz merged commit dc70208 into centos-automotive-suite:main Apr 16, 2026
4 checks passed
@bennyz bennyz deleted the worktree-fix-flash-oci-prefix branch April 16, 2026 16:43
@coderabbitai coderabbitai Bot mentioned this pull request Apr 29, 2026
10 tasks
@bennyz bennyz restored the worktree-fix-flash-oci-prefix branch June 1, 2026 08:36
@bennyz bennyz deleted the worktree-fix-flash-oci-prefix branch June 1, 2026 11:14
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