- Notifications
You must be signed in to change notification settings - Fork905
feat(agent/agentcontainers): refactor Lister to ContainerCLI and implement new methods#18243
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
2984d4e toae52380Compare5cf4ae7 to2dc395dCompare…ement new methodsThis change redefines the Lister interface and implements new methodsneeded for sub agent injection.Updatescoder/internal#621
2dc395d toea0125dCompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks good to me although have left a comment about theExecAs tests
Uh oh!
There was an error while loading.Please reload this page.
| // 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👀 this is wild, do you have an example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The username is passwd dependent, so there are various ways this could happen. Simple demo:
FROM busyboxRUN rm /etc/passwd
❯docker run -it --rm --user roottest whoamidocker: Error response from daemon: unable to find user root: no matching entries in passwd fileRun 'docker run --help' for more information
Compared to
❯docker run -it --rm --user 0test whoamiwhoami: unknown uid 0
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Idea, non-blocking: could we also allow passing in anio.Reader instead that we convert to TAR archive format and stream tostdin /-?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Uh oh!
There was an error while loading.Please reload this page.
a12429e intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
This change redefines the Lister interface and implements new methods
needed for sub agent injection.
Updatescoder/internal#621