fix(image): gracefully handle missing Docker/Podman engine#539
Merged
ruromero merged 2 commits intoJun 25, 2026
Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideImageUtils.hostInfo() is changed to return an empty string instead of throwing when the Docker/Podman engine is missing or returns only error output, and the corresponding unit test is updated to assert the new behavior. Sequence diagram for ImageUtils.hostInfo graceful fallbacksequenceDiagram
participant Caller
participant ImageUtils
participant Operations
Caller->>ImageUtils: hostInfo(engine, info)
ImageUtils->>Operations: getCustomPathOrElse(engine)
Operations-->>ImageUtils: exec
ImageUtils->>Operations: runProcessGetFullOutput(null, cmd, null)
alt IOException during runProcessGetFullOutput
Operations-->>ImageUtils: RuntimeException(cause=IOException)
ImageUtils-->>Caller: ""
else Successful process execution
Operations-->>ImageUtils: ProcessExecOutput
alt output.getOutput().isEmpty() and (output.getError() not empty or exitCode != 0)
ImageUtils-->>Caller: ""
else Normal output
ImageUtils-->>Caller: output
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Using an empty string as the sentinel for "engine unavailable" makes it hard to distinguish from a legitimate empty
docker infooutput; consider returning anOptional<String>or a dedicated result type instead so callers can explicitly distinguish error from content. - Catching a generic
RuntimeExceptionand then inspectinggetCause()forIOExceptionis a bit brittle and obscures the real failure mode; if possible, narrow the exception type at theOperations.runProcessGetFullOutputboundary (e.g., by exposing checked exceptions or a domain-specific exception) so this method can handle only the intended error cases.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Using an empty string as the sentinel for "engine unavailable" makes it hard to distinguish from a legitimate empty `docker info` output; consider returning an `Optional<String>` or a dedicated result type instead so callers can explicitly distinguish error from content.
- Catching a generic `RuntimeException` and then inspecting `getCause()` for `IOException` is a bit brittle and obscures the real failure mode; if possible, narrow the exception type at the `Operations.runProcessGetFullOutput` boundary (e.g., by exposing checked exceptions or a domain-specific exception) so this method can handle only the intended error cases.
## Individual Comments
### Comment 1
<location path="src/main/java/io/github/guacsec/trustifyda/image/ImageUtils.java" line_range="288-289" />
<code_context>
+ try {
+ output = Operations.runProcessGetFullOutput(null, cmd, null);
+ } catch (RuntimeException e) {
+ if (e.getCause() instanceof IOException) {
+ return "";
+ }
+ throw e;
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider preserving error information (e.g., via logging) before returning an empty string on IO failures.
This behavior makes IO failures indistinguishable from a successful call that just returns no data, which complicates debugging and monitoring. If you want to avoid throwing, please at least log the exception (including the message), or use a distinct sentinel/status object so callers can tell the difference between “no info available” and “failed to get info.”
Suggested implementation:
```java
Operations.ProcessExecOutput output;
try {
output = Operations.runProcessGetFullOutput(null, cmd, null);
} catch (RuntimeException e) {
if (e.getCause() instanceof IOException) {
LOGGER.warn("Failed to run '{}' info command due to I/O error", exec, e.getCause());
return "";
}
throw e;
}
```
To compile successfully you also need to ensure that:
1. The class defines a logger named `LOGGER`, for example:
`private static final org.slf4j.Logger LOGGER = org.slf4j.LoggerFactory.getLogger(ImageUtils.class);`
2. If SLF4J is not the logging framework used in this project, replace `LOGGER.warn(...)` with the appropriate logging call consistent with the rest of the codebase.
</issue_to_address>
### Comment 2
<location path="src/test/java/io/github/guacsec/trustifyda/image/ImageUtilsTest.java" line_range="551-552" />
<code_context>
- var exception =
- assertThrows(RuntimeException.class, () -> ImageUtils.hostInfo("docker", "info"));
- assertEquals("test-error", exception.getMessage());
+ var result = ImageUtils.hostInfo("docker", "info");
+ assertEquals("", result);
}
}
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test ensuring non-IO-related RuntimeExceptions are still propagated
The new test covers the empty-string behavior when error output is returned, but it doesn’t verify that non‑IO `RuntimeException`s are still propagated.
Please add a dedicated test (e.g., `test_host_info_other_runtime_exceptions_propagate`) that mocks `Operations.runProcessGetFullOutput` to throw a `RuntimeException` with a non‑IO cause (or no cause), and uses `assertThrows` to confirm `ImageUtils.hostInfo` rethrows it. This will clearly distinguish expected graceful failures from unexpected errors and protect against regressions.
Suggested implementation:
```java
var result = ImageUtils.hostInfo("docker", "info");
assertEquals("", result);
}
@Test
void test_host_info_other_runtime_exceptions_propagate() {
var runtimeException = new RuntimeException("non-io-error");
// Configure Operations.runProcessGetFullOutput to throw a non-IO-related RuntimeException
// following the same argument pattern as the other hostInfo tests.
operations
.when(
() ->
Operations.runProcessGetFullOutput(
eq("docker"), isNull(), aryEq(new String[] {"docker", "info"}), isNull()))
.thenThrow(runtimeException);
var thrown =
assertThrows(RuntimeException.class, () -> ImageUtils.hostInfo("docker", "info"));
// Ensure the same RuntimeException instance is propagated
assertSame(runtimeException, thrown);
}
}
```
To make this compile and integrate correctly with the rest of the test file, you will also need to ensure:
1. The existing static mock object for `Operations` is named `operations` and is in scope for this test (the new test assumes the same mock field/variable used by the other hostInfo tests).
2. The following static imports are present at the top of the file (if they are not already):
- `import static org.junit.jupiter.api.Assertions.assertThrows;`
- `import static org.junit.jupiter.api.Assertions.assertSame;`
- `import static org.mockito.ArgumentMatchers.eq;`
3. If the test class uses a different pattern for mocking (e.g., `try (MockedStatic<Operations> operations = mockStatic(Operations.class)) { ... }` inside each test instead of a shared `operations` field), adjust the new test to follow that pattern:
- Wrap the mocking and assertions in the same `try (MockedStatic<Operations> operations = mockStatic(Operations.class)) { ... }` structure.
- Use the same argument matcher combination (`eq`, `isNull`, `aryEq`, etc.) as the existing hostInfo tests.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #539 +/- ##
=======================================
Coverage ? 68.72%
Complexity ? 1000
=======================================
Files ? 65
Lines ? 4253
Branches ? 744
=======================================
Hits ? 2923
Misses ? 990
Partials ? 340
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ImageUtils.hostInfo() now returns an empty string instead of throwing RuntimeException when the container engine binary is missing (IOException) or returns error output. This allows callers to fall back gracefully rather than crashing when neither Docker nor Podman is installed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tion Address Sourcery review feedback: log a warning when a container engine is not available instead of silently returning empty, and add a test verifying that non-IO RuntimeExceptions still propagate. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ruromero
force-pushed
the
fix/docker-podman-graceful-fallback
branch
from
95be102 to
f493385
Compare
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
3 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.
Summary
ImageUtils.hostInfo()now returns an empty string instead of throwingRuntimeExceptionwhen the container engine binary is missing (IOException) or returns error outputTest plan
ImageUtilsTestsuite🤖 Generated with Claude Code
Summary by Sourcery
Handle missing container engine binaries gracefully when retrieving image host info.
Bug Fixes:
Tests: