-
Notifications
You must be signed in to change notification settings - Fork 35
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
Add Docker driver config to keep container around #139
Conversation
Currently, it is hard to debug the state of an invocation image after the various files and environment variables have been inserted by the runtime. By not cleaning up the container, we allow bundle developers an easier way to debug problems. The parameter is CLEANUP_CONTAINERS, default is true. Fixes cnabio#128
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.
LGTM, thanks.
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.
LGTM, thanks @astrieanna
if !ok { | ||
settings["CLEANUP_CONTAINERS"] = "true" | ||
} else if value != "true" && value != "false" { | ||
fmt.Printf("CLEANUP_CONTAINERS environment variable has unexpected value %q. Supported values are 'true', 'false', or unset.", value) |
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 the cnab-go runtime shouldn't print something on the stdout without a way to disable it. SetConfig
should either return an error (would maybe break things), or ignore invalid values and consider them as the default (aka: "true"). WDYT?
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'd prefer to be able to return an error message, but I didn't want to change the interface for all the drivers just to add one option to one driver.
I strongly dislike UIs that involve the user setting an environment variable and then getting zero feedback if their setting is being ignored. If you have to read the code to understand why a feature doesn't seem to be working for you, then that is not a good user experience.
There are a few ways to fix this that I'd be happy with, but they all require changes to the driver interface/contract:
SetConfig
could return anerror
, which would allow UX feedback for invalid config.- The drivers could take a stream to write their logs/errors to, and Duffle can decide whether to pass in stdout/stderr or /dev/null based on user config.
- The drivers could all use the same logging library as Duffle, which should allow Duffle to configure global settings like log-level.
@silvin-lubecki WDYT?
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.
@astrieanna I'd be in favor of modifying SetConfig
so it can return an error 👍
But I also think we need a better way to log everything happening in the driver. That's a pain point we have in docker app
.
@@ -55,11 +55,20 @@ func (d *Driver) Config() map[string]string { | |||
"PULL_ALWAYS": "Always pull image, even if locally available (0|1)", | |||
"DOCKER_DRIVER_QUIET": "Make the Docker driver quiet (only print container stdout/stderr)", | |||
"OUTPUTS_MOUNT_PATH": "Absolute path to where Docker driver can create temporary directories to bundle outputs. Defaults to temp dir.", | |||
"CLEANUP_CONTAINERS": "If true, the docker container will be destroyed when it finishes running. If false, it will not be destroyed. The supported values are true and false. Defaults to true.", |
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.
Maybe we should invert the name and the logic, with something like "KEEP_CONTAINERS" ? So by default it's false
and I guess the code in SetConfig
would be simpler?
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.
Can you open a follow up issue for that Silvin?
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.
Done here #141
Currently, it is hard to debug the state of an invocation image after
the various files and environment variables have been inserted by the
runtime. By not cleaning up the container, we allow bundle developers
an easier way to debug problems.
The parameter is CLEANUP_CONTAINERS, default is true. I decided to log some feedback if the value is incorrect. This is not what the other settings do, but seems user-friendly.
Fixes #128
cc @youreddy