cli/command/container: copyToContainer: improve error-handling#6254
cli/command/container: copyToContainer: improve error-handling#6254thaJeztah merged 2 commits intodocker:masterfrom
Conversation
Make it more clearly identifiable where we're dealing with the named error-return Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
The logic used in this function was confusing; some errors were ignored, but responses handled regardless. The intent here is to try to detect whether the destination exists inside the container and is of the right "type" (otherwise produce an error). Failing to "stat" the path in the container means we can't produce a nice error for the user, but we'll continue the request, which either would succeed or produce an error returned by the daemon. While working on this patch, I noticed that some error-handling on the daemon side is incorrect. This patch does not fix those cases, but makes the logic slightly easier to follow (we should consider extracting the "stat" code to a separate function though). Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
There was a problem hiding this comment.
Pull Request Overview
This PR improves error handling and code clarity in the copyToContainer function within the Docker CLI. The changes focus on making error handling more explicit and improving the logic flow when dealing with container path validation.
Key changes:
- Removes named error return to improve clarity of error handling flow
- Restructures stat error handling to be more explicit about when errors are intentionally ignored
- Adds explicit error acknowledgment for intentionally ignored stat operations
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| return fmt.Errorf(`destination "%s:%s" must be a directory or a regular file: %w`, copyConfig.container, dstPath, err) | ||
| } | ||
| dstInfo.Path = linkTarget | ||
| dstStat, err = apiClient.ContainerStatPath(ctx, copyConfig.container, linkTarget) |
There was a problem hiding this comment.
The error from the second ContainerStatPath call is not handled. If this call fails, the subsequent code at line 332 will still execute with a potentially invalid dstStat, which could cause issues in ValidateOutputPathFileMode.
| dstStat, err = apiClient.ContainerStatPath(ctx, copyConfig.container, linkTarget) | |
| dstStat, err = apiClient.ContainerStatPath(ctx, copyConfig.container, linkTarget) | |
| if err != nil { | |
| // Ignore any error and assume that the parent directory of the destination | |
| // path exists, in which case the copy may still succeed. If there is any | |
| // type of conflict (e.g., non-directory overwriting an existing directory | |
| // or vice versa) the extraction will fail. If the destination simply did | |
| // not exist, but the parent directory does, the extraction will still | |
| // succeed. | |
| _ = err // Intentionally ignore stat errors (see above) | |
| return nil | |
| } |
There was a problem hiding this comment.
READ THE BLOODY COMMENT @copilot - it's not hard, really, you should try it!
|
CoPilot is not entirely wrong; the current logic is still somewhat confusing, and I'm planning to extract it to a separate function, but still want to do some more digging into some parts before moving it 😂 I'll bring this one in, as at least we got rid of the confusing named error-return and my IDE no longer is screaming about unhandled errors. |
cli/command/container: copyToContainer rename error-return
Make it more clearly identifiable where we're dealing with the
named error-return
cli/command/container: copyToContainer: improve error-handling
The logic used in this function was confusing; some errors were ignored,
but responses handled regardless. The intent here is to try to detect
whether the destination exists inside the container and is of the right
"type" (otherwise produce an error).
Failing to "stat" the path in the container means we can't produce a
nice error for the user, but we'll continue the request, which either
would succeed or produce an error returned by the daemon.
While working on this patch, I noticed that some error-handling on the
daemon side is incorrect. This patch does not fix those cases, but
makes the logic slightly easier to follow (we should consider extracting
the "stat" code to a separate function though).
- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)