Skip to content
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

code review discussions #8

Merged
merged 2 commits into from
May 27, 2021
Merged

code review discussions #8

merged 2 commits into from
May 27, 2021

Conversation

davidmdm
Copy link
Collaborator

A continuation of the docker cli branch, pointing back into the docker cli branch.
Restructure docker CLI file and have working commands.

But lots of stuff to discuss!!!!!!!!

@davidmdm davidmdm requested a review from silphid May 26, 2021 21:30
Copy link
Owner

@silphid silphid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great job! very lean and mean! ;)

output, err := cmd.Output()
if err != nil {
return nil, err
}
return getOutputLines(output), nil
return strings.Split(string(output), "\n"), nil
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sur Windows, les séparateurs de lignes sont "\r\n", si on veut être multi-plateforme... C'est pour ça que j'avais fait getOutputLines()

}

func startContainer(ctx context.Context, containerName string, cmd []string) error {
args := append([]string{"start", "-i", containerName}, cmd...)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docker start does not accept command name/args at the end, I fixed that

@silphid silphid merged commit 21b6c11 into docker-cli May 27, 2021
@silphid silphid deleted the docker-cli-continued branch May 27, 2021 00:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants