Export stack commands #1519
Export stack commands #1519
Conversation
silvin-lubecki
force-pushed
the
export-stack-commands
branch
from
bbf87ac
to
afda9ac
Compare
Codecov Report
@@ Coverage Diff @@
## master #1519 +/- ##
==========================================
+ Coverage 55.16% 55.22% +0.06%
==========================================
Files 301 301
Lines 20384 20377 -7
==========================================
+ Hits 11244 11253 +9
+ Misses 8335 8318 -17
- Partials 805 806 +1 |
There was a problem hiding this comment.
@silvin-lubecki I guess it's required for docker/app or things like that ? 👼
silvin-lubecki
force-pushed
the
export-stack-commands
branch
from
afda9ac
to
f1dff4e
Compare
|
Yep good guess @vdemeester 😄 |
Factorize orchestrator switch among stack commands. Signed-off-by: Silvin Lubecki <[email protected]>
silvin-lubecki
force-pushed
the
export-stack-commands
branch
from
f1dff4e
to
cf0271a
Compare
|
@simonferquel @vdemeester PTAL, I rebased and cleaned this PR 🦁 |
|
LGTM |
There was a problem hiding this comment.
lgtm, but see my comment inline; I'm not sure this is fully the right direction to go in
@vdemeester any suggestions?
| @@ -35,7 +35,8 @@ func newListCommand(dockerCli command.Cli, common *commonOptions) *cobra.Command | |||
| return cmd | |||
| } | |||
|
|
|||
| func runList(cmd *cobra.Command, dockerCli command.Cli, opts options.List, orchestrator command.Orchestrator) error { | |||
| // RunList performs a stack list against the specified orchestrator | |||
| func RunList(cmd *cobra.Command, dockerCli command.Cli, opts options.List, orchestrator command.Orchestrator) error { | |||
There was a problem hiding this comment.
Only concern I have with exporting all of these is that;
- These functions now become a public "API", with no interface defined for them
- this can complicate making changes in the CLI (they were kept non-exported, as their signature/interface was not "stable")
- Some of these functions actually have a "UX" component built-in (e.g. they print confirmation messages on stdout/stderr, have an option to print formatted output); is that really suitable for external consumption?
- calling these commands would become almost equivalent to
exec.Cmd("docker stack ls")
- calling these commands would become almost equivalent to
For the underlying code on the other hand, we have various interfaces defined; if those are lacking functionality that's needed, should we review those interfaces, and make that the API to use?
There was a problem hiding this comment.
For the underlying code on the other hand, we have various interfaces defined; if those are lacking functionality that's needed, should we review those interfaces, and make that the API to use?
I guess you can see these functions as a Facade, a simple API endpoint for a simple use, but if you have a more advanced use case, you can still rely on the underlying interfaces (like if you only want to talk to Swarm and don't care about Kubernetes). Otherwise if you want the same behavior, you need to replicate this complicated switch kubernetes/swarm.
Some of these functions actually have a "UX" component built-in (e.g. they print confirmation messages on stdout/stderr, have an option to print formatted output); is that really suitable for external consumption?
calling these commands would become almost equivalent to exec.Cmd("docker stack ls")
That is exactly the purpose of this PR: someone who vendors the docker/cli can benefit the same look-and-feel (and behavior) as the "real" docker-cli binary, without relying on the binary itself.
These functions now become a public "API", with no interface defined for them
this can complicate making changes in the CLI (they were kept non-exported, as their signature/interface was not "stable")
I think these signatures are simple and somehow stable. I don't have a crystal ball but my 2 cts is that they won't be modified before a long time.
There was a problem hiding this comment.
So, I do agree with @thaJeztah and also @silvin-lubecki 😅 🙃
I think our client API is too low-level, and we need to export those because of that 😅
We may define better APIs in the future (that would not have cobra.Command or comand.Cli in the method signature) for example.
I think it's the less worse we can do for now 👼 so, still LGTM 👼
There was a problem hiding this comment.
was discussing with @chris-crone to add a note somewhere (perhaps the readme) to indicate that these functions, even though they're exported, should not be considered a stable API (as in; their signature can change, and shouldn't be relied on)
We could also add notes to the GoDoc of these, similar to https://golang.org/pkg/testing/#RunTests
An internal function but exported because it is cross-package; part of the implementation of the "go test" command.
But that would be a bit more work to do
There was a problem hiding this comment.
was discussing with @chris-crone to add a note somewhere (perhaps the readme) to indicate that these functions, even though they're exported, should not be considered a stable API (as in; their signature can change, and shouldn't be relied on)
Will do that in a follow-up 👍
- What I did
Export stack commands for vendoring and extract/factorize common code.
- A picture of a cute animal (not mandatory but encouraged)