Detect single-value advanced syntax#2243
Conversation
silvin-lubecki
left a comment
silvin-lubecki
left a comment
There was a problem hiding this comment.
LGTM thank you @zappy-shu 👍
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
Thanks! left two minor nits, also, I'd recommend removing the fixes xxx from the commit message of the first commit. In this particular case, it probably won't cause much harm, but GitHub is quite "eager" to close issues based on commit messages. Putting fixes in the commit message itself can cause issues to be closed prematurely if someone in another repository merges the commit (particularly annoying if the "fix" doesn't take effect until it's vendored in another repository);
Fixes #2058 allowing the use of the advanced source=x syntax for config and secret values when there is no comma
Generally, I only add the fixes in the PR description on GitHub, but not in the commit message for that reason
perhaps the first and last commit should be squashed (as the last one fixes up tests that were added in the first one); keeping the second one separate looks good to me (because it's a change in behavior, and to make it stand out better)
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { |
There was a problem hiding this comment.
Can you change this to
| t.Run(tc.name, func(t *testing.T) { | |
| tc := tc | |
| t.Run(tc.name, func(t *testing.T) { |
see moby/moby#40115 (comment) (and https://blog.golang.org/subtests)
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { |
There was a problem hiding this comment.
same here
| t.Run(tc.name, func(t *testing.T) { | |
| tc := tc | |
| t.Run(tc.name, func(t *testing.T) { |
see moby/moby#40115 (comment) (and https://blog.golang.org/subtests)
thaJeztah
added
impact/changelog
status/2-code-review
and removed
status/0-triage
labels
Allow the use of the advanced source=x syntax for config and secret values when there is no comma Before this change the following would fail with config not found: docker service create --name hello1 --config source=myconfig nginx:alpine And the following would fail with secret not found: docker service create --name hello2 --secret source=mysecret nginx:alpine Signed-off-by: Nick Adcock <[email protected]>
When using advanced syntax for setting config and secret values, default the target value to the source value when the user does not specify a target. Signed-off-by: Nick Adcock <[email protected]>
Refactors the config and secret unit tests to be table driven to remove duplication Signed-off-by: Nick Adcock <[email protected]>
d91b50a to
3baa6d5
Compare
4 participants
- What I did
Adds the ability to just specify the
sourcefor config and secret flags using the advanced syntax as per https://docs.docker.com/engine/reference/commandline/service_create/#create-a-service-with-secrets. When the target is not specified the target is set to the source, same as when using the simple syntax.- How I did it
When checking if the user is using the simple syntax check if the field contains an
=as well as checking if there is more than one field.- How to verify it
Unit tests have been added for just setting the source for both secrets and configs (all existing test cases for secrets have been copied for configs as well).
To manually test, follow the steps in #2058
- Description for the changelog
Fixes #2058 allowing the use of the advanced syntax when setting a config or secret with only the source field.
- A picture of a cute animal (not mandatory but encouraged)