Conversation
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 18 minutes and 38 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughAdds OCI image flashing support to the QEMU driver: new async Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant QemuFlasherClient as QemuFlasherClient
participant QemuFlasher as QemuFlasher
participant Fls as FLS_Subprocess
participant Env as Environment
Client->>QemuFlasherClient: flash("oci://..."), partition
QemuFlasherClient->>QemuFlasher: streamingcall("flash_oci", oci_url, partition)
QemuFlasher->>Env: validate URL & resolve credentials
QemuFlasher->>Env: get_fls_binary(fls_version, allow_custom, custom_url)
QemuFlasher->>Fls: exec("fls from-url <oci_url> <target_path>"), env(with creds)
loop stream
Fls-->>QemuFlasher: stdout_chunk / stderr_chunk
QemuFlasher-->>QemuFlasherClient: (stdout_chunk, stderr_chunk, None)
QemuFlasherClient-->>Client: forward chunks
end
Fls-->>QemuFlasher: returncode
QemuFlasher-->>QemuFlasherClient: ("", "", returncode)
QemuFlasherClient-->>Client: final result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
bennyz
added
the
build-pr-images/jumpstarter
Container ImagesThe following container images have been built for this PR:
Images expire after 7 days. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
python/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/client.py (1)
13-23: Expose OCI credentials on the public client API instead of only readingos.environ.Right now private-registry auth is only configurable through
OCI_USERNAME/OCI_PASSWORD, so SDK callers cannot pass credentials per request and concurrent flashes against different registries have to mutate process-global state. Since the driver already acceptsoci_username/oci_password,QemuClient.flash_oci()should forward them explicitly and keep env lookup, if you still want it, at the CLI boundary.♻️ Possible shape
- def flash_oci(self, oci_url: str, partition: str | None = None): + def flash_oci( + self, + oci_url: str, + partition: str | None = None, + oci_username: str | None = None, + oci_password: str | None = None, + ): """Flash an OCI image to the specified partition using fls. @@ - return self.flasher.flash(oci_url, target=partition) + return self.flasher.call("flash_oci", oci_url, partition, oci_username, oci_password)Based on learnings, "The TFTP driver ... client.py should remain simple as it delegates these responsibilities to the driver."
Also applies to: 47-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/client.py` around lines 13 - 23, The flash method currently only reads OCI credentials from environment variables and never forwards per-call credentials; update QemuClient.flash to accept and forward oci_username and oci_password parameters (or read env as a fallback) when calling self.call("flash_oci", ...) so callers can pass credentials per request; specifically, change the signature/usage in flash to pass oci_username and oci_password into the flash_oci RPC (instead of only using os.environ) and preserve the existing env lookup only as a fallback (or move env-only behavior to the CLI layer), making sure the call to flash_oci uses the explicit oci_username/oci_password values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@python/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py`:
- Around line 287-304: The test test_flash_oci_no_credentials currently only
checks mock_run.env when the test process already lacks FLS_REGISTRY_*; seed the
ambient env first and assert flasher.flash_oci strips them: before calling
flasher.flash_oci, use patch.dict(os.environ, {"FLS_REGISTRY_USERNAME": "x",
"FLS_REGISTRY_PASSWORD": "y"}, clear=False) (or equivalent) to inject
credentials into the process environment, then call flasher.flash_oci and verify
mock_run.call_args[1]["env"] does not contain "FLS_REGISTRY_USERNAME" or
"FLS_REGISTRY_PASSWORD"; update the test function test_flash_oci_no_credentials
to perform this seeding and the same assertions around flasher.flash_oci and
mock_run.
In `@python/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py`:
- Around line 102-105: The environment for the flash subprocess (fls_env) should
not inherit any existing FLS_REGISTRY_USERNAME/FLS_REGISTRY_PASSWORD from
os.environ; update the code in the driver (around fls_env creation and anywhere
similar at the other block) to explicitly pop or delete "FLS_REGISTRY_USERNAME"
and "FLS_REGISTRY_PASSWORD" from the copied env before using it, and only set
those keys when oci_username and oci_password are provided (i.e., in the fls_env
setup for flash_oci and the similar block at lines ~110-116, ensure you remove
existing keys when credentials are None and only inject keys when non-None).
---
Nitpick comments:
In `@python/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/client.py`:
- Around line 13-23: The flash method currently only reads OCI credentials from
environment variables and never forwards per-call credentials; update
QemuClient.flash to accept and forward oci_username and oci_password parameters
(or read env as a fallback) when calling self.call("flash_oci", ...) so callers
can pass credentials per request; specifically, change the signature/usage in
flash to pass oci_username and oci_password into the flash_oci RPC (instead of
only using os.environ) and preserve the existing env lookup only as a fallback
(or move env-only behavior to the CLI layer), making sure the call to flash_oci
uses the explicit oci_username/oci_password values.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5d1b0e8b-7fb1-453f-bb8e-ee6a50fea8bc
📒 Files selected for processing (3)
python/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/client.pypython/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.pypython/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py
mangelajo
reviewed
Apr 15, 2026
Comment on lines
+13
to
+23
| def flash(self, path, *, target=None, operator=None, compression=None): | ||
| if isinstance(path, str) and path.startswith("oci://"): | ||
| oci_username = os.environ.get("OCI_USERNAME") | ||
| oci_password = os.environ.get("OCI_PASSWORD") | ||
|
|
||
| if bool(oci_username) != bool(oci_password): | ||
| raise ValueError("OCI authentication requires both OCI_USERNAME and OCI_PASSWORD environment variables") | ||
|
|
||
| return self.call("flash_oci", path, target, oci_username, oci_password) | ||
|
|
||
| return super().flash(path, target=target, operator=operator, compression=compression) |
Member
There was a problem hiding this comment.
if we do the bifurcation on the driver side it will be easier with a plan that @kirkbrauer was proposing to allow multi-language clients :)
Comment on lines
+94
to
+98
| fls_binary = get_fls_binary( | ||
| fls_version=self.parent.fls_version, | ||
| fls_binary_url=self.parent.fls_custom_binary_url, | ||
| allow_custom_binaries=self.parent.fls_allow_custom_binaries, | ||
| ) |
Member
There was a problem hiding this comment.
I would go ahead and start installing the fls binary on the jumpstarter container, I was exactly looking for using fls... :)
Member
Author
There was a problem hiding this comment.
Comment on lines
+110
to
+116
| result = subprocess.run( | ||
| fls_cmd, | ||
| capture_output=True, | ||
| text=True, | ||
| check=True, | ||
| env=fls_env, | ||
| timeout=self.parent.flash_timeout, |
Member
There was a problem hiding this comment.
I would do that later, but we need to find a standard way to stream back the output of the shell process (we can steal this from the shell driver) :)
but not now, don't want to complicate your PR
bennyz
added
build-pr-images/jumpstarter
bennyz
force-pushed
the
qemu-oci
branch
6 times, most recently
from
b0dbb2f to
5aa3d86
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
python/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py (1)
310-326:⚠️ Potential issue | 🟡 MinorSeed stale
FLS_REGISTRY_*vars in the no-credentials test.Line 325 asserts
env is None, which meansflsinherits any ambientFLS_REGISTRY_USERNAME/FLS_REGISTRY_PASSWORD. Seed those vars and assert they are not propagated, otherwise stale registry credentials can affect public flashes unnoticed.🧪 Suggested hardening
- # Ensure OCI env vars are not set so driver doesn't pick them up - env_clean = {k: v for k, v in os.environ.items() if k not in ("OCI_USERNAME", "OCI_PASSWORD")} + # Ensure OCI env vars are not set and stale fls credentials are stripped. + env_clean = { + k: v + for k, v in os.environ.items() + if k not in ("OCI_USERNAME", "OCI_PASSWORD", "FLS_REGISTRY_USERNAME", "FLS_REGISTRY_PASSWORD") + } + env_clean["FLS_REGISTRY_USERNAME"] = "stale-user" + env_clean["FLS_REGISTRY_PASSWORD"] = "stale-pass" with patch.dict(os.environ, env_clean, clear=True): with patch("jumpstarter_driver_qemu.driver.get_fls_binary", return_value="fls"): with patch( "asyncio.create_subprocess_exec", new_callable=AsyncMock, return_value=mock_process ) as mock_exec: await _collect_flash_oci(flasher, "oci://quay.io/public/image:tag") env = mock_exec.call_args.kwargs["env"] - assert env is None + assert "FLS_REGISTRY_USERNAME" not in env + assert "FLS_REGISTRY_PASSWORD" not in envThis still needs the driver to pass a sanitized env instead of
None. As per coding guidelines, "Each driver package must include comprehensive tests indriver_test.py."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py` around lines 310 - 326, The test test_flash_oci_no_credentials currently asserts env is None which lets ambient FLS_REGISTRY_USERNAME/FLS_REGISTRY_PASSWORD leak; modify the test to seed those FLS_REGISTRY_* variables into the parent environment (e.g., include FLS_REGISTRY_USERNAME and FLS_REGISTRY_PASSWORD in the patched os.environ), call _collect_flash_oci(flasher,...), capture mock_exec = the patched asyncio.create_subprocess_exec and inspect mock_exec.call_args.kwargs["env"], and assert that this env either is a sanitized mapping that does not contain FLS_REGISTRY_USERNAME or FLS_REGISTRY_PASSWORD (preferred) or, if the driver returns None, explicitly assert that those keys are not present in the subprocess environment propagation; reference test_flash_oci_no_credentials, _collect_flash_oci, flasher, and asyncio.create_subprocess_exec when making the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@python/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py`:
- Around line 597-610: The test test_flash_oci_convenience_method currently only
verifies the URL argument and can pass if partition is ignored; update the test
to assert the partition is forwarded to the flasher invocation by checking
mock_exec.call_args (e.g., assert mock_exec.call_args.args[3] == "bios" or
another appropriate index/value) so Qemu.flash_oci("oci://...",
partition="bios") actually results in the partition being passed through to the
subprocess call.
- Around line 410-423: The test test_flash_oci_partial_credentials_rejected is
flaky because the implementation fills missing explicit
oci_username/oci_password from environment variables; update the test to ensure
no ambient OCI_USERNAME/OCI_PASSWORD are present before calling
QemuFlasher.flash_oci (e.g., monkeypatch or temporarily unset os.environ keys)
so the partial-credential calls reliably raise ValueError, referencing the Qemu
class, its child flasher (QemuFlasher) and the flash_oci method when locating
where to change the test.
---
Duplicate comments:
In
`@python/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py`:
- Around line 310-326: The test test_flash_oci_no_credentials currently asserts
env is None which lets ambient FLS_REGISTRY_USERNAME/FLS_REGISTRY_PASSWORD leak;
modify the test to seed those FLS_REGISTRY_* variables into the parent
environment (e.g., include FLS_REGISTRY_USERNAME and FLS_REGISTRY_PASSWORD in
the patched os.environ), call _collect_flash_oci(flasher,...), capture mock_exec
= the patched asyncio.create_subprocess_exec and inspect
mock_exec.call_args.kwargs["env"], and assert that this env either is a
sanitized mapping that does not contain FLS_REGISTRY_USERNAME or
FLS_REGISTRY_PASSWORD (preferred) or, if the driver returns None, explicitly
assert that those keys are not present in the subprocess environment
propagation; reference test_flash_oci_no_credentials, _collect_flash_oci,
flasher, and asyncio.create_subprocess_exec when making the changes.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 82d72432-46a9-4193-9b92-a8d12931aef8
📒 Files selected for processing (5)
.github/workflows/python-tests.yamlpython/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/client.pypython/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.pypython/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.pypython/pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- python/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/client.py
- python/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py
Contributor
There was a problem hiding this comment.
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 `@python/Dockerfile`:
- Around line 17-20: get_fls_github_url() currently maps the aarch64 asset to
"fls-aarch64-linux" which mismatches the Dockerfile naming
("fls-aarch64-linux-musl"); update the aarch64 cases in get_fls_github_url (the
two places referenced) to use "fls-aarch64-linux-musl" so the generated GitHub
download URL matches the binary preinstalled by the Dockerfile (adjust the asset
name logic for ARCH == "aarch64" to include the "-musl" suffix).
In `@python/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.py`:
- Around line 108-115: The current logic independently fills oci_username and
oci_password from environment variables which can mix an explicit argument with
a stale env value; change it so environment fallback only happens when both
oci_username and oci_password were not provided (both falsy), otherwise do not
read env and instead raise ValueError if only one of oci_username/oci_password
was supplied. Update the credential-loading block around oci_username and
oci_password (the same scope that checks bool(oci_username) !=
bool(oci_password)) so it first checks if both are falsy and only then reads
OCI_USERNAME/OCI_PASSWORD from os.environ, then keep the existing paired-check
to raise when exactly one is present.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fcd8823a-fde1-48a0-a51b-21414d14b219
📒 Files selected for processing (6)
.github/workflows/python-tests.yamlpython/Dockerfilepython/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/client.pypython/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver.pypython/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.pypython/pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (4)
- python/pyproject.toml
- .github/workflows/python-tests.yaml
- python/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/client.py
- python/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-opus-4.6
…ests Change --cov to --cov=. in pytest addopts so coverage.xml includes an absolute source root, allowing diff-cover to correctly match coverage data to git diff paths in the monorepo. Without this, diff-cover fell back to low-coverage data from dependent packages that only import the module. Remove bare --cov from CI PYTEST_ADDOPTS to prevent cross-package coverage contamination. The bare flag caused every package test run to record coverage for all imported modules, polluting diff-cover results. Add tests for the inner wait_for timeout handler and process cleanup on early generator exit in _stream_subprocess. Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-opus-4.6
Update Dockerfile and get_fls_github_url() to use fls-aarch64-linux-musl binary name, matching the actual fls release asset naming convention. Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.