Conversation
fixes #1101 additional context: * this was introduced in docker 1.13 (1/2017), we require docker 17.10 (10/2017), this should not have any issues dependency-wise, as `docker-init` is in the docker install from that point in time. unless explicitly removed, it should be in the dind container we use as well... * the PR that introduced this to docker is moby/moby#26061 for additional context * it may be wise to put this through some paces, if anybody has any... interesting... function containers. the tests seem to work fine, however, and this shouldn't be something users have to think about (?) at all, just something that we are doing. this isn't the default in docker for compatibility reasons, which is maybe a yellow flag but I am not sure tbh
kmjohansen
approved these changes
Sep 4, 2018
kmjohansen
left a comment
kmjohansen
left a comment
There was a problem hiding this comment.
Looks good; thanks for implementing this.
Member
|
Works fine on mac , LGTM |
zootalures
approved these changes
Sep 4, 2018
Contributor
Author
|
thanks for review... see if any windows people come out of the woodwork we can flip it relatively easily |
Member
|
Docker4Windows uses basically the same LK VM images as d4m so I "expect it will be fine" |
Contributor
Author
famous last words! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes #1101
additional context:
(10/2017), this should not have any issues dependency-wise, as
docker-initis in the docker install from that point in time. unless explicitly removed,
it should be in the dind container we use as well...
Add init process for zombie fighting and signal handling moby/moby#26061 for additional context
interesting... function containers. the tests seem to work fine, however, and
this shouldn't be something users have to think about (?) at all, just
something that we are doing. this isn't the default in docker for
compatibility reasons, which is maybe a yellow flag but I am not sure tbh
issues it should be easy to flip it back off without having to deal with a build, etc.
we need to test on osx and win and we can default it to 'off' and turn it on
for any 'serious' deployments if there are issues to keep the out of the box xp working,
it does seem to work on osx (per linked issue above), but i haven't tested that or win myself.
it seems to add about 50ms to the start time, which seems worth it.