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 --template-driver option for secrets/configs #896

Merged
merged 1 commit into from
Feb 22, 2018
Merged

Add --template-driver option for secrets/configs #896

merged 1 commit into from
Feb 22, 2018

Conversation

thaJeztah
Copy link
Member

relates to moby/moby#33702

dnephin
dnephin approved these changes Feb 21, 2018
View reviewed changes
Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

LGTM

cli/command/config/create_test.go Outdated
return types.ConfigCreateResponse{}, errors.Errorf("expected name %q, got %q", name, spec.Name)
}

if !reflect.DeepEqual(spec.Templating.Name, expectedDriver.Name) {
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this just a comparing a string? so it could use != ?

Copy link
Member Author

Choose a reason for hiding this comment

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

hm, yes, copy-pasta'd from TestSecretCreateWithDriver. Let me update

@codecov-io
Copy link

codecov-io commented Feb 21, 2018 •
edited
Loading

Codecov Report

Merging #896 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #896      +/-   ##
==========================================
+ Coverage   53.23%   53.25%   +0.02%     
==========================================
  Files         258      258              
  Lines       16347    16357      +10     
==========================================
+ Hits         8702     8711       +9     
- Misses       7081     7082       +1     
  Partials      564      564

@thaJeztah
Copy link
Member Author

Thinking if templating-driver or template-driver is the best name; let me know if template-driver is a better name

cpuguy83
cpuguy83 reviewed Feb 21, 2018
View reviewed changes
cli/command/config/create_test.go Outdated
@@ -105,3 +105,32 @@ func TestConfigCreateWithLabels(t *testing.T) {
assert.NoError(t, cmd.Execute())
assert.Equal(t, "ID-"+name, strings.TrimSpace(cli.OutBuffer().String()))
}

func TestConfigCreateWithTemplatingDriver(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Test for config create without driver?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's a test above that doesn't have a driver, is that what you meant?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should check and make sure Templating is nil in that case, I think?

Copy link
Contributor

Choose a reason for hiding this comment

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

This would be done already if the test used DeepEqual on the entire spec instead of looking at specific fields. Maybe we should do that here as well.

@dnephin
Copy link
Contributor

dnephin commented Feb 21, 2018

I like template-driver

@thaJeztah
Copy link
Member Author

Changed to template-driver

@thaJeztah thaJeztah changed the title Add --templating-driver option for secrets/configs Add --template-driver option for secrets/configs Feb 21, 2018
@thaJeztah
Copy link
Member Author

hmf, secrets side is failing;

Error: expected {Annotations:{Name:foo Labels:map[]} Data:[115 101 99 114 101 116 95 102 111 111 95 98 97 114 10] Driver:<nil> Templating:<nil>}, got {Annotations:{Name:foo Labels:map[]} Data:[115 101 99 114 101 116 95 102 111 111 95 98 97 114 10] Driver:<nil> Templating:<nil>}

But I don't see a different:

{Annotations:{Name:foo Labels:map[]} Data:[115 101 99 114 101 116 95 102 111 111 95 98 97 114 10] Driver:<nil> Templating:<nil>}
{Annotations:{Name:foo Labels:map[]} Data:[115 101 99 114 101 116 95 102 111 111 95 98 97 114 10] Driver:<nil> Templating:<nil>}

@thaJeztah
Add --template-driver option for secrets/configs
d11b5cc 
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
cpuguy83
cpuguy83 approved these changes Feb 22, 2018
View reviewed changes
Copy link
Collaborator

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

vdemeester
vdemeester approved these changes Feb 22, 2018
View reviewed changes
Copy link
Collaborator

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐯

@vdemeester vdemeester merged commit ef7d8be into docker:master Feb 22, 2018
@GordonTheTurtle GordonTheTurtle added this to the 18.03.0 milestone Feb 22, 2018
@thaJeztah thaJeztah deleted the templated-configs-secrets branch February 22, 2018 09:30
@thaJeztah thaJeztah mentioned this pull request Feb 26, 2018
Merged
@gbarr01 gbarr01 mentioned this pull request Mar 12, 2018
Open
@albers albers mentioned this pull request Apr 14, 2018
Merged
@thaJeztah thaJeztah mentioned this pull request May 30, 2018
Merged
@vdemeester vdemeester mentioned this pull request May 31, 2018
Closed
2 tasks
@thaJeztah thaJeztah mentioned this pull request Jul 2, 2018
Closed
This was referenced Mar 28, 2019
Merged
Merged
Merged
Merged
@thaJeztah thaJeztah mentioned this pull request Jun 10, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dnephin dnephin dnephin approved these changes

@vdemeester vdemeester vdemeester approved these changes

@cpuguy83 cpuguy83 cpuguy83 approved these changes

Assignees
No one assigned
Labels
docs/revisit impact/changelog process/cherry-picked status/2-code-review
Projects
None yet
Milestone
18.03.0
Development

Successfully merging this pull request may close these issues.
6 participants
@thaJeztah @codecov-io @dnephin @vdemeester @cpuguy83 @GordonTheTurtle