feat(agent/agentcontainers): refactor Lister to ContainerCLI and implement new methods #18243
Conversation
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
This was referenced Jun 5, 2025
2984d4e to
ae52380
Compare
Base automatically changed from
mafredri/feat-agent-devcontainer-injection-1
to
main
June 5, 2025 13:58
5cf4ae7 to
2dc395d
Compare
…ement new methods This change redefines the Lister interface and implements new methods needed for sub agent injection. Updates coder/internal#621
2dc395d to
ea0125d
Compare
DanielleMaywood
approved these changes
Jun 6, 2025
johnstcn
approved these changes
Jun 6, 2025
Comment on lines
+560
to
+562
| // UID 0 is more portable than the name root, so we use that | ||
| // because some containers may not have a user named "root". | ||
| altUID = "0" |
There was a problem hiding this comment.
👀 this is wild, do you have an example?
There was a problem hiding this comment.
The username is passwd dependent, so there are various ways this could happen. Simple demo:
FROM busybox
RUN rm /etc/passwd❯ docker run -it --rm --user root test whoami
docker: Error response from daemon: unable to find user root: no matching entries in passwd file
Run 'docker run --help' for more informationCompared to
❯ docker run -it --rm --user 0 test whoami
whoami: unknown uid 0
Comment on lines
+546
to
+552
| func (dcli *dockerCLI) Copy(ctx context.Context, containerName, src, dst string) error { | ||
| _, stderr, err := runCmd(ctx, dcli.execer, "docker", "cp", src, containerName+":"+dst) | ||
| if err != nil { | ||
| return xerrors.Errorf("copy %s to %s:%s: %w: %s", src, containerName, dst, err, stderr) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Idea, non-blocking: could we also allow passing in an io.Reader instead that we convert to TAR archive format and stream to stdin / -?
There was a problem hiding this comment.
I like the idea, although I don't see a benefit currently for its planned usage. If we have use-cases where we don't want to write files to disk it would definitely be the go-to approach.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This change redefines the Lister interface and implements new methods
needed for sub agent injection.
Updates coder/internal#621