-
Notifications
You must be signed in to change notification settings - Fork 0
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
First draft of CLI docker integration #7
Conversation
src/internal/docker/cli.go
Outdated
yey "github.com/silphid/yey/src/internal" | ||
) | ||
|
||
type CLI struct{} | ||
|
||
func (c CLI) Start(ct yey.Context, imageTag, containerName string) error { | ||
return fmt.Errorf("not implemented") | ||
func NewCLI() CLI { |
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.
Question I have been wondering. Do we need an empty type? Do we need a yey.Docker interface that this CLI type needs to implement?
Basically the question becomes, are we injecting the this implementation somewhere? Are we planning on mocking the results of CLI[Method] ?
Would these tests add value?
I am not sure of this, but I wouldn't mind simply having the docker package export functions that wrap os/exec calls,
and having integration tests against actual docker versions as a first pass.
Food for thought.
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.
effectivement, je vois pas le besoin du empty struct ou interface à court terme, juste pour les tests éventuellement, mais on peut l'enlever pour l'instant
src/internal/docker/cli.go
Outdated
if err != nil { | ||
return err | ||
} | ||
args := []string{"run"} |
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 think it might be clearer if instead of modified arg slices, we had smaller docker building blocks, like a run
function and a start
function. that way it could simply be:
if running {
return containerExec(containerName, ...)
}
return containerRun(containerName, ...)
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.
Oui, ça pourrait.
src/internal/docker/cli.go
Outdated
// Common args | ||
args := []string{"-it", "--env LS_COLORS", "--env TERM", "--env TERM_COLOR", "--env TERM_PROGRAM"} | ||
if !isExec { | ||
args = append(args, "--rm", "--name", containerName) |
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.
TODO Discuss the philosophy of --rm
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.
Y'a tellement de flags docker possibles et j'aimerais mieux pas tous les hardcoder si possible (genre "--network=host" qui est très pratique, mais pas nécessairement ce que tout le monde veut). Alors, je pensais pouvoir configurer des flags explicitement dans le contexte. Comme ça, tout le monde pourrait contrôler ces flags qui sont plus personnels.
} | ||
|
||
// Context env vars | ||
for name, value := range yeyCtx.Env { |
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.
Isn't the environment only needed to be passed at run
? Shouldn't exec into a container that was run with an env keep the env that was set previously?
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.
c'est une question que je me demandais, à tester quand on aura un moment
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.
je pense qu'en faisaint un split entre exec et run tu vas ne meme pas avoir besoin de cette fonction apres, car tout c'est args la sont seulement vraiment important quand tu fais run. Et donc tu n'auras pas besoin d'une function a part comme celle ci.
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.
je viens de vérifier et les env vars sont persistants across docker run
, exec
et start
.
No description provided.