Few refactoring around the docker provider #1440
Conversation
vdemeester
requested review from
errm,
timoreimann,
SantoDE,
Russell-IO,
emilevauge and
dtomcej
| } | ||
|
|
||
| // dockerData holds the need data to the Docker provider | ||
| // dockerData holds the need data to the Provider p |
| op(c) | ||
| } | ||
|
|
||
| return *c |
There was a problem hiding this comment.
How about returning a pointer since we are already applying the ops on the pointer?
There was a problem hiding this comment.
So, we always use ContainerJSON as not a pointer so… I prered to have just on return *c than a lot of *containerJSON(…) 😝
There was a problem hiding this comment.
My point was that we pass-by-pointer when we apply the ops only to do a return-by-value eventually. But I suppose it's fine this way.
| return func(s *network.EndpointSettings) { | ||
| s.IPAddress = ip | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm on the fence whether the builder deserves its on set if tests. It doesn't seem strictly required but would be helpful to have them.
There was a problem hiding this comment.
right, so… I don't want to overdo it too. 😛
| ) | ||
|
|
||
| func containerJSON(ops ...func(*docker.ContainerJSON)) docker.ContainerJSON { | ||
| c := &docker.ContainerJSON{ |
There was a problem hiding this comment.
variable names with one letter, are you sure ?
There was a problem hiding this comment.
not sure, but between c, ctnr or cont, I went lazy 👼 (didn't want to use container because it would override the container package)
There was a problem hiding this comment.
Wow cool @vdemeester 👍
Small comment but other than that, LGTM
provider/provider.go
Outdated
| @@ -60,7 +61,7 @@ func (p *BaseProvider) getConfiguration(defaultTemplateFile string, funcMap temp | |||
| var defaultFuncMap = template.FuncMap{ | |||
| "replace": replace, | |||
| "tolower": strings.ToLower, | |||
| "normalize": normalize, | |||
| "Normalize": Normalize, | |||
There was a problem hiding this comment.
I suppose that a regression might have been introduced here as no template have been modified. We should leave the first one in low caps to stay consistent "normalize": Normalize.
There was a problem hiding this comment.
Oups you are right. I'll fix that ;)
There was a problem hiding this comment.
Shouldn't this have been caught by some test?
There was a problem hiding this comment.
I think it is not used in any template (in the repo) but.. Yeah.. 😅
vdemeester
force-pushed
the
docker-provider-refacto
branch
2 times, most recently
from
48a134c to
51a53b3
Compare
vdemeester
force-pushed
the
docker-provider-refacto
branch
from
5bd7091 to
d835b4a
Compare
Makes it simpler to manage :) Signed-off-by: Vincent Demeester <[email protected]>
- Split the file into smaller ones (docker, swarm and service tests) - Use some builder to reduce a little bit the noise for creating containers Signed-off-by: Vincent Demeester <[email protected]>
vdemeester
force-pushed
the
docker-provider-refacto
branch
from
d835b4a to
b04ba36
Compare
|
LGTM ⚓️ NICE WORK @vdemeester |
This is a big one, sorry for that 😛 (and this is just the beginning). This one does 2 things, separated in 2 commits
dockerprovider to its own package (provider/docker). I intend to do that for all of the providers.Note that there is still some flakyness in the unit tests 😱
🦁