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

simple fix for #405 and #406 #488

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

delabassee
Copy link
Contributor

Simple fix that uses docker inspect to check that Docker is running and that the init-image exists.

@rdallman
Copy link
Contributor

is the pull step before this? lazy and didn't see it in the diff (sorry). users shouldn't need to manually pull the init image and docker inspect will burp if the image isn't downloaded locally (which was the point I think? got that) -- docker build itself will pull the image, i'm assuming we were just relying on that, which makes sense to me. docker info is a quick way to check that docker is running. docker search is a quick way to check that an image exists on a registry. I guess here we have to balance that some users may build their own init images locally and have them and some users may be relying on them to exist on a registry to be pulled at build time (like ours, presumably, eg for newer users on their first run through). do we need to try both search and inspect to get the desired validation behavior or what exactly do we need to validate here?

@delabassee
Copy link
Contributor Author

I changed this to simply handle docker run potential failures.

commands/init.go Outdated Show resolved Hide resolved
@delabassee
Copy link
Contributor Author

I think that all cases are properly handled (init-image not found, docker down, docker not in path, untar failing). I couldn't make error handling fails.

_, errStdout = io.Copy(stdout, stdoutIn)
_, errStderr = io.Copy(stderr, stderrIn)

errUntar := untarStream(bytes.NewReader(stdoutBuf.Bytes()))
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry for delay here. we can use the io.Pipe and not need to buffer here - this is a good idea because images are on the order of megabytes. we don't actually use stderr it seems so i'm thinking something like:

tarIn, tarOut := io.Pipe()
cmd := exec.Command("...")
cmd.Stderr = ioutil.Discard // honestly, os.Stderr makes sense, we can do w/e
cmd.Stdout = tarOut
err := cmd.Start()
if err != nil {
  return err
}
err = untarStream(tarIn)
if err != nil {
  return err // can format error about init image here, add stderr if we want
}
err = cmd.Wait()
if err != nil {
  return err // can format error about init image here, add stderr if we want
}

this pretty much expects that untar stream will fuck up if the command fucks up. if for some reason it doesn't, then we can get the exit code out of Wait(). in the happy path we expect untar to just work, and Wait() to return a nil error. I think that this makes sense, but double check me :) wdyt?

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.

3 participants