search

rafecolton / docker-builder

Docker builder builds Docker images from a friendly config file.
MIT License
80 stars 11 forks source link

Fix bug where server cannot be started w/o basic auth #136

Closed rafecolton closed 9 years ago

rafecolton commented 9 years ago

and add corresponding integration tests to avoid future regression

fixes https://github.com/rafecolton/docker-builder/issues/134

/cc @wjzijderveld

meatballhat commented 9 years ago

Exposing the integration test mode as a flag feels weird to me. I would find it more idiomatic to have the server handle SIGQUIT or maybe SIGUSR1 to achieve the desired behavior.

rafecolton commented 9 years ago

@meatballhat thanks for the feedback! It didn't feel quite right to me either, so having that confirmed by a second opinion means refactor time!

rafecolton commented 9 years ago

@meatballhat mind taking a look again?

meatballhat commented 9 years ago

I'm assuming this works but I don't understand how. The signal handling is in a test_ file, which should only be compiled into the .test binary, but the tests exec the docker-builder binary. Huh. Beyond that, what I'd intended by my earlier comment was that the production binary should have special handling for SIGUSR1, plus it should probably also handle SIGQUIT and SIGHUP, but that's another issue :smiley_cat:

rafecolton commented 9 years ago

Ah, it works because only *_test.go files are special, not test_*.go.

I went back and forth on whether or not to include the SIGUSR1-handling file in the production binary and ultimately decided that it felt safer not to, though I'm not convinced it makes a huge difference either way. Currently, it is only included when built with -tags integration.

As for other signals, currently, docker-builder handles SIGINT and SIGTERM via gocleanup. I have decided not to handle SIGHUP thus far because I leave demonization up to the parent process (i.e. docker via upstart). As for SIGQUIT, I say if somebody really wants a core dump, let 'em have it :smile:

rafecolton commented 9 years ago

Also worth noting that afaik there is no Go equivalent trapping EXIT like there is in bash. The same effect is achieved through gocleanup by calling gocleanup.Exit(<code>) which calls all of the gocleanup handler functions before exiting.

meatballhat commented 9 years ago

In all, this is mostly about feelings, which are built up from exposure to other stuff, so it's very unlikely you'll find a solution that "feels" right to all parties involved. :shipit: