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 "name" field to secrets and configs to allow interpolation in Compose files #668

Merged
merged 3 commits into from Nov 22, 2017

Conversation

Projects
None yet
6 participants
@ilyasotkov
Contributor

ilyasotkov commented Nov 6, 2017

See issue #667

In Compose file version 3.4, volumes gained the "name" field which allowed the volumes' names to be interpolated for simple version bumps using environmental variables.

That feature is also needed for secrets and configs. Proposed usage:

version: "3.5" # future Compose file version

secrets:
  my-secret:
    name: my-secret-${VERSION} # name of the Docker Secret with variable interpolation
    file: ./secret.html

configs:
  my-config:
    name: my-config-${VERSION} # same thing for configs
    file: ./default.conf

services:
  web:
    image: nginx:1.13.6-alpine
    ports:
      - "80:80"
    secrets:
      - source: my-secret # reference by the name of the key, consistent with how it is for volumes now in 3.4
        target: /usr/share/nginx/html/index.html
    configs:
      - source: my-config
        target: /etc/nginx/conf.d/default.conf

What I did: I just followed the way the feature is already implemented for volumes. I also bumped the Compose file schema version to 3.5.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Nov 6, 2017

Codecov Report

Merging #668 into master will increase coverage by 0.06%.
The diff coverage is 71.87%.

@@            Coverage Diff             @@
##           master     #668      +/-   ##
==========================================
+ Coverage   51.28%   51.34%   +0.06%     
==========================================
  Files         216      216              
  Lines       17743    17761      +18     
==========================================
+ Hits         9099     9120      +21     
+ Misses       8175     8172       -3     
  Partials      469      469

codecov-io commented Nov 6, 2017

Codecov Report

Merging #668 into master will increase coverage by 0.06%.
The diff coverage is 71.87%.

@@            Coverage Diff             @@
##           master     #668      +/-   ##
==========================================
+ Coverage   51.28%   51.34%   +0.06%     
==========================================
  Files         216      216              
  Lines       17743    17761      +18     
==========================================
+ Hits         9099     9120      +21     
+ Misses       8175     8172       -3     
  Partials      469      469

@ilyasotkov ilyasotkov changed the title from Add "name" field to secrets and configs to allow interpolation in Compose files. to Add "name" field to secrets and configs to allow interpolation in Compose files Nov 6, 2017

@docker docker deleted a comment from GordonTheTurtle Nov 6, 2017

@dnephin

Thanks!

Left some comments. I'm going to open a PR to fix some existing issues with the volume implementation of this. I'll also look at see if we can remove some duplication between secrets/configs.

Show outdated Hide outdated cli/compose/convert/compose.go Outdated
Show outdated Hide outdated cli/compose/loader/loader.go Outdated
Show outdated Hide outdated cli/compose/convert/service.go Outdated
@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Nov 6, 2017

Collaborator

Opened #670 to fix the warnings on volume. For LoadSecrets and LoadConfigObjs you could pass in the configDetails since they already accept a field from that struct.

#670 also made a change so that external.name is never read

Collaborator

dnephin commented Nov 6, 2017

Opened #670 to fix the warnings on volume. For LoadSecrets and LoadConfigObjs you could pass in the configDetails since they already accept a field from that struct.

#670 also made a change so that external.name is never read

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Nov 6, 2017

Collaborator

I opened #671 to remove a lot of the duplication between secrets and configs. This PR is going to conflict quite a bit. You might want to rebase on top of #671 so that you don't want to make all the changes twice (once for secrets, and once for configs).

Collaborator

dnephin commented Nov 6, 2017

I opened #671 to remove a lot of the duplication between secrets and configs. This PR is going to conflict quite a bit. You might want to rebase on top of #671 so that you don't want to make all the changes twice (once for secrets, and once for configs).

@GordonTheTurtle GordonTheTurtle added dco/no and removed dco/no labels Nov 10, 2017

@ilyasotkov

This comment has been minimized.

Show comment
Hide comment
@ilyasotkov

ilyasotkov Nov 10, 2017

Contributor

So I rewrote the PR on top of #671 to remove some of the duplication between configs / secrets and fixed some issues the original PR had as well.

I also added a couple of tests for the functionality, but I wasn't sure if duplicating secrets and configs there is the right way to go.

Contributor

ilyasotkov commented Nov 10, 2017

So I rewrote the PR on top of #671 to remove some of the duplication between configs / secrets and fixed some issues the original PR had as well.

I also added a couple of tests for the functionality, but I wasn't sure if duplicating secrets and configs there is the right way to go.

@dnephin

looks good, couple more small things.

Also looks like there is a lint and validate failure

Show outdated Hide outdated cli/compose/convert/compose.go Outdated
Show outdated Hide outdated cli/compose/loader/loader.go Outdated
@ilyasotkov

This comment has been minimized.

Show comment
Hide comment
@ilyasotkov

ilyasotkov Nov 11, 2017

Contributor

Updated.

Contributor

ilyasotkov commented Nov 11, 2017

Updated.

@docker docker deleted a comment from GordonTheTurtle Nov 13, 2017

@dnephin

LGTM

Thanks!

@ilyasotkov

This comment has been minimized.

Show comment
Hide comment
@ilyasotkov

ilyasotkov Nov 22, 2017

Contributor

So, what else needs to be done to get this merged? 👋

Contributor

ilyasotkov commented Nov 22, 2017

So, what else needs to be done to get this merged? 👋

@vdemeester

LGTM 🐯
@ilyasotkov needs a rebase though

ilyasotkov added some commits Nov 10, 2017

Add tests for secret.name in compose files.
Signed-off-by: Ilya Sotkov <ilya@sotkov.com>
Add secret.name and config.name in compose.
Signed-off-by: Ilya Sotkov <ilya@sotkov.com>
Remove duplication for secrets / configs, combine 3.5 loader tests, r…
…emove extraneous error call, regenerate schema correctly

Signed-off-by: Ilya Sotkov <ilya@sotkov.com>
@ilyasotkov

This comment has been minimized.

Show comment
Hide comment
@ilyasotkov

ilyasotkov Nov 22, 2017

Contributor

Rebased

Contributor

ilyasotkov commented Nov 22, 2017

Rebased

@dnephin dnephin merged commit 8d41ba0 into docker:master Nov 22, 2017

8 checks passed

ci/circleci: cross Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: shellcheck Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
ci/circleci: validate Your tests passed on CircleCI!
Details
codecov/patch 71.87% of diff hit (target 50%)
Details
codecov/project 51.34% (+0.06%) compared to 2d0e2d2
Details
dco-signed All commits are signed

@GordonTheTurtle GordonTheTurtle added this to the 17.12.0 milestone Nov 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment