kubernetes/conversion_test: use test-builders package #2180
Conversation
| } | ||
| } | ||
|
|
||
| // TODO convertToServices currently doesn't set swarm.EndpointSpec.Ports |
There was a problem hiding this comment.
Added a TODO for this; looks like the conversion doesn't return the ports in the spec for the service
internal/test/builders/service.go
Outdated
|
|
||
| // Service creates a service with default values. | ||
| // Any number of service builder functions can be passed to augment it. | ||
| // Currently, only ServiceName is implemented |
internal/test/builders/service.go
Outdated
| } | ||
| service := &swarm.Service{} | ||
| ServiceID("serviceID") | ||
| ServiceName("defaultServiceName") |
There was a problem hiding this comment.
I don't understand this part. You call a function that returns a function and then you discard its result, meaning you never call the function returned.
Should be something like
ServiceID("serviceID")(service)
ServiceName("defaultServiceName")(service)There was a problem hiding this comment.
Oh, lol, think I forgot to push a change 😂
| o(&s) | ||
| } | ||
| return s | ||
| options := append([]func(*swarm.Service){}, ServiceID(id), ServiceName(name), ServiceImage("image")) |
There was a problem hiding this comment.
Appending to an empty slice does not make sense to me. Easier to write as
options := []func(*swarm.Service){ServiceID(id), ServiceName(name), ServiceImage("image")}
silvin-lubecki
left a comment
silvin-lubecki
left a comment
There was a problem hiding this comment.
LGTM with @kolyshkin changes 👍
Also rewrite `Service()` to use the available options Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
refactor_conversion_test
branch
from
2c711f5 to
b80fd0c
Compare
Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
refactor_conversion_test
branch
from
b80fd0c to
2d0c10d
Compare
| }, | ||
| } | ||
| service := &swarm.Service{} | ||
| defaults := []func(*swarm.Service){ServiceID("serviceID"), ServiceName("defaultServiceName")} |
There was a problem hiding this comment.
Actually thinking that we could even remove these defaults; looks like it's currently not used in other locations where the code depends on it being set (which means we could convert this package to something that can be used outside of testing as well
|
@silvin-lubecki updated; PTAL |
4 participants
also some small changes to the internal/test/builders
relates to #2173