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.

Sign up for GitHub

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

Jump to bottom

Add Docker driver config to keep container around #139

Merged
merged 1 commit into from
Oct 3, 2019
Merged

Add Docker driver config to keep container around #139

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion driver/docker/docker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.",
Copy link
Contributor

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?

Copy link
Member

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Done here #141

}
}

// SetConfig sets Docker driver configuration
func (d *Driver) SetConfig(settings map[string]string) {
// Set default and provide feedback on acceptable input values.
value, ok := settings["CLEANUP_CONTAINERS"]
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)
Copy link
Contributor

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?

Copy link
Contributor Author

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:

  1. SetConfig could return an error, which would allow UX feedback for invalid config.
  2. 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.
  3. 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?

Copy link
Contributor

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.

}

d.config = settings
}

Expand Down Expand Up @@ -175,7 +184,9 @@ func (d *Driver) exec(op *driver.Operation) (driver.OperationResult, error) {
return driver.OperationResult{}, fmt.Errorf("cannot create container: %v", err)
}

defer cli.Client().ContainerRemove(ctx, resp.ID, types.ContainerRemoveOptions{})
if d.config["CLEANUP_CONTAINERS"] == "true" {
defer cli.Client().ContainerRemove(ctx, resp.ID, types.ContainerRemoveOptions{})
}

tarContent, err := generateTar(op.Files)
if err != nil {
Expand Down