chore(container): delegate FromID to FindContainerByID and add unit tests for errors#132
Open
xHozey wants to merge 3 commits intodocker:mainfrom
Open
chore(container): delegate FromID to FindContainerByID and add unit tests for errors#132xHozey wants to merge 3 commits intodocker:mainfrom
xHozey wants to merge 3 commits intodocker:mainfrom
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors container.FromID to centralize container ID lookup by delegating to SDKClient.FindContainerByID, instead of calling ContainerList and filtering within the container package.
Changes:
- Replaced
ContainerList+ manual filtering inFromIDwith a call toFindContainerByID. - Simplified
FromIDto only build aContainerfrom the returned summary viaFromResponse.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
xHozey
changed the title
mdelapenya
reviewed
Feb 17, 2026
Comment on lines
+50
to
+53
| ctr, err := container.Run(context.Background(), container.WithImage("alpine:latest")) | ||
| require.NoError(t, err) | ||
|
|
||
| cli := ctr.Client() |
Member
There was a problem hiding this comment.
bug: I think this container is going to be running forever. Not an issue on CI because of the ephemeral nature of the workers. I think we can get a new fresh client without the ctr.
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.
What does this PR do?
This PR refactors the FromID helper to delegate container lookup to FindContainerByID instead of manually calling ContainerList and applying filters internally.
It also adds unit tests for error cases, including empty container ID and non-existent container ID, to ensure proper handling of failure scenarios.
As a result, the error messages and types returned by FromID may differ slightly:
Errors are now wrapped with "find container by ID" prefixes.
The empty-containerID case now returns an invalid-argument error from FindContainerByID rather than going through ContainerList.
Why is it important?
This change reduces duplication and centralizes container ID lookup logic in a single place. By relying on FindContainerByID, we ensure consistent filtering behavior across the SDK and make future maintenance easier.
If the lookup implementation changes in the future, it only needs to be updated in one location. This improves code clarity and maintainability without modifying the public API or core functionality.
How to test this PR
inside container package in container.from_test.go run TestFromID function