Add `--env-file` flag to `docker create service` by yongtang · Pull Request #24844 · moby/moby

yongtang · 2016-07-20T14:39:33Z

- What I did

This fix tries to address the issue in #24712 and add --env-file file to docker create service.

- How I did it

The flag has been added and related documentation has been updated.

- How to verify it

An additional integration has been added.

- Description for the changelog

Add --env-file flag to docker create service.

- A picture of a cute animal (not mandatory but encouraged)

This fix fixes #24712.

Signed-off-by: Yong Tang [email protected]

thaJeztah · 2016-07-20T16:17:31Z

Thanks @yongtang

For reference, this was not yet decided on; we may want to make a design document, describing which flags make sense for services (and which not); also that some flags are very platform specific, so may need a more "generic" approach so that they can be used in a cross-platform Swarm (win/linux/solaris?)

yongtang · 2016-07-20T16:21:45Z

Thanks @thaJeztah. No worries. Just keep me posted.

jadbox · 2016-07-21T01:02:33Z

@thaJeztah While I'm impartial, I just wanted to further make a point on this. This particular flag seems critical for practical use of services. Every company that I have experience with uses "--env-file" for starting their containers as their microservices use a non-trivial number of environment vars that are not ideal to bake into the image itself.

Until this feature is added, I'll need to either use this PR or create a env-file-to-parameters cli generator that wraps docker service create.

thaJeztah · 2016-07-21T09:41:00Z

@jadbox thanks for the feedback; yes, I can see it being useful, but we're getting really close to GA, and still have a lot of work to do. I understand the current implementation is not perfect, but should be doable.

I want to prevent making "quick" decisions, and being stuck with those. For example, maybe we should take #12997 into account (or at least discuss it)

cpuguy83 · 2016-09-08T13:38:27Z

I guess this is fine being that it's a client-only feature.
I hate env-file since everyone disagrees in the formatting... but we already support it on docker run.

thaJeztah · 2016-09-14T10:53:26Z

Generally, I'm ok with this as well, some thoughts:

do we need to make sure that the key/value pairs as read from the env-file are sorted?
what plans do we have to implement this for docker service update as well? And;
what would users expect to happen if I do docker service update ...., and changes were made to the env-file? We don't actually store the reference to the env-file location in the service definition, so the file will only be used during docker service create, however, what if I want to update some values in the file, and update the service?

yongtang · 2016-09-15T05:02:13Z

@thaJeztah Currently docker service update support:

      --env-add value                  Add or update environment variables (default [])
      --env-rm value                   Remove an environment variable (default [])

So I feel like having something like --env-file-add or --env-file-rm might not be very concise and may make things complicated. I tend to think --env-file as not a true individual flag, but just a helper of

--env A=B --env C=D --env E=F ...

so that user does not need to pass many --env in one line.

From that angle, I am not sure we want to have --env-file support in docker service update as I assume changes in docker service update will not be big.

But that is just my personal opinion. Let me know if we want to support --env-file for service update.

thaJeztah · 2016-09-15T09:44:54Z

@yongtang

So I feel like having something like --env-file-add or --env-file-rm might not be very concise and may make things complicated

I would be definitely against a --env-file-add option, because we don't store the env-file reference. Simply --env-file could be an option for docker update, with a warning in the documentation that it will replace all existing env-vars on the service. I can see this being beneficial for updating all env-vars with the latest changes, or (for example) switching to a new configuration.

@cpuguy83 @jadbox wdyt ^^ ?

with regard to:

what would users expect to happen if I do docker service update ...., and changes were made to the env-file?

Possibly that's a matter of properly documenting what the --env-file option does, i.e. that it's just an easier way to set multiple --env options, but does not preserve a reference to the name/path of the env-file itself.

thaJeztah · 2016-09-15T18:15:39Z

Ok, we discussed this in the maintainers meeting, and we think just adding it to create for now, is ok

thaJeztah · 2016-09-29T14:33:21Z

cli/command/service/opts.go

Should env-variables be sorted and duplicates removed here ?

Thanks @thaJeztah. I updated the PR and duplicates of the env entries have been removed. I leave the order alone (not sorted), as that may potentially be different behavior from docker run. But let me know if we want to change both.

My thought behind this was that in services, a change in order can result in the service being re-deployed. Not sure if that's important in this case, but worth checking.

thaJeztah · 2016-09-29T22:33:03Z

cli/command/service/opts.go

Hm, having the same key twice, would result in the first value being overwritten by the second one, so wondering if we should keep both, or simply de-duplicate by key

Thanks @thaJeztah, the PR has been updated with duplicated keys removed.

thaJeztah · 2016-10-21T17:09:13Z

cli/command/service/opts.go

We discussed this and think it's better to sort by key, and not trying to preserve the original sort order. This would make sure that the env-vars are always sorted the same way (and duplicates are already removed earlier, so that's no problem)

Actually, with env, we need to preserve order.

@stevvooe order is preserved before removing the duplicates (see the loop above), do you think we should not do that?

stevvooe · 2016-10-21T18:55:23Z

cli/command/service/opts.go

So, the algorithm for env replacement is simple but inefficient (really):

for _, env := newEnv { // need to process each var, in order k, _ := splitEnv(env) for i, current := currentEnv { // remove duplicates if current == env { continue next // no update required, may hide this behind flag to preserve order or newEnv } if strings.HasPrefix(current, k+"=") { currentEnv = append(currentEnv[:i], currentEnv[i+1:]...) } } currentEnv = append(currentEnv, env) }

In practice, N is small enough to keep this from being too slow. There are probably ways to make this more efficient.

It preserves the order of the new and old set, while replacing duplicates. It also assumes that there are no existing duplicates.

thaJeztah · 2016-10-26T18:05:09Z

ping @yongtang could you update / have a look at @stevvooe's comment? We hope to get this into 1.13

yongtang · 2016-10-26T19:57:21Z

Thanks @thaJeztah @stevvooe for the review. The PR has been updated. Please take a look and let me know if there are any issues.

tiborvass · 2016-10-27T18:30:26Z

cli/command/service/opts.go

currentEnv := make([]string, 0, len(envVariables))

Thanks @tiborvass. The PR has been updated.

This fix tries to address the issue in 24712 and add `--env-file` file to `docker create service`. Related documentation has been updated. An additional integration has been added. This fix fixes 24712. Signed-off-by: Yong Tang <[email protected]>

tiborvass · 2016-10-27T20:26:29Z

LGTM

thaJeztah

LGTM (code and docs)

ping @albers @sdurrheimer as this needs an update to the completion scripts

GordonTheTurtle added the status/0-triage label Jul 20, 2016

yongtang mentioned this pull request Jul 20, 2016

[1.12] Docker create service --env-file #24712

Closed

yongtang force-pushed the 24712-create-service-env-file branch from 369631e to 575ebf5 Compare July 20, 2016 15:29

thaJeztah added status/1-design-review and removed status/0-triage labels Jul 20, 2016

yongtang force-pushed the 24712-create-service-env-file branch from 575ebf5 to 76528c9 Compare July 21, 2016 02:19

yongtang force-pushed the 24712-create-service-env-file branch 2 times, most recently from 4c581af to 109af59 Compare September 9, 2016 01:28

thaJeztah added status/2-code-review and removed status/1-design-review labels Sep 15, 2016

yongtang force-pushed the 24712-create-service-env-file branch from 109af59 to fd2c3e3 Compare September 17, 2016 22:28

thaJeztah mentioned this pull request Sep 29, 2016

Support --env-file flag in docker service create #27003

Closed

thaJeztah reviewed Sep 29, 2016

View reviewed changes

yongtang force-pushed the 24712-create-service-env-file branch from fd2c3e3 to 73d40c3 Compare September 29, 2016 21:17

thaJeztah reviewed Sep 29, 2016

View reviewed changes

yongtang force-pushed the 24712-create-service-env-file branch 2 times, most recently from 30a5ec2 to e1b263c Compare October 3, 2016 09:10

yongtang force-pushed the 24712-create-service-env-file branch from e1b263c to 3cff8a4 Compare October 17, 2016 11:52

thaJeztah requested changes Oct 21, 2016

View reviewed changes

stevvooe reviewed Oct 21, 2016

View reviewed changes

yongtang force-pushed the 24712-create-service-env-file branch from 3cff8a4 to bd2607f Compare October 22, 2016 16:02

thaJeztah added this to the 1.13.0 milestone Oct 26, 2016

yongtang force-pushed the 24712-create-service-env-file branch from bd2607f to a550868 Compare October 26, 2016 19:56

yongtang force-pushed the 24712-create-service-env-file branch from a550868 to bd9217b Compare October 27, 2016 16:11

tiborvass requested changes Oct 27, 2016

View reviewed changes

yongtang force-pushed the 24712-create-service-env-file branch from bd9217b to ee3105c Compare October 27, 2016 18:56

tiborvass approved these changes Oct 27, 2016

View reviewed changes

thaJeztah approved these changes Oct 27, 2016

View reviewed changes

thaJeztah merged commit 322f88f into moby:master Oct 27, 2016

thaJeztah added impact/changelog impact/cli labels Oct 27, 2016

yongtang deleted the 24712-create-service-env-file branch October 27, 2016 20:42

sdurrheimer mentioned this pull request Oct 28, 2016

Add zsh completion for 'docker service create --env-file' #27850

Merged

albers mentioned this pull request Oct 28, 2016

Add bash completion for docker service create --env-file #27856

Merged

thaJeztah mentioned this pull request Nov 7, 2016

[epic] add more options to service create / service update #25303

Open

Conversation

yongtang commented Jul 20, 2016

Uh oh!

thaJeztah commented Jul 20, 2016

Uh oh!

yongtang commented Jul 20, 2016

Uh oh!

jadbox commented Jul 21, 2016

Uh oh!

thaJeztah commented Jul 21, 2016

Uh oh!

cpuguy83 commented Sep 8, 2016

Uh oh!

thaJeztah commented Sep 14, 2016

Uh oh!

yongtang commented Sep 15, 2016

Uh oh!

thaJeztah commented Sep 15, 2016

Uh oh!

thaJeztah commented Sep 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevvooe Oct 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Oct 26, 2016

Uh oh!

yongtang commented Oct 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tiborvass commented Oct 27, 2016

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

stevvooe Oct 21, 2016 •

edited

Loading