Make several CLI commands testable #86
Conversation
pcalcado
force-pushed
the
phil/49-conduit-when-pod-doesnt-exist
branch
from
db7cb4e to
c49c7e7
Compare
| var versionCmd = &cobra.Command{ | ||
| Use: "version", | ||
| Short: "Print the client and server version information", | ||
| Long: "Print the client and server version information.", | ||
| Args: cobra.NoArgs, | ||
| Run: exitSilentlyOnError(func(cmd *cobra.Command, args []string) error { | ||
| fmt.Println("Client version: " + controller.Version) |
There was a problem hiding this comment.
Previously if something went wrong with k8s/server, we'd get output like this:
$ conduit version
Client version: v0.0.1
Server version: unavailable
$ echo $?
2
... with this change conduit version now prints nothing:
$ conduit version
$ echo $?
2
I'd rather know the client version, and get some feedback that something's not working. This is similar to how kubectl works.
There was a problem hiding this comment.
@siggy
It still prints the version:
$ conduit version
Client version: v0.1.0
Server version: v0.1.0
$ bin/go-run cli version
Client version: v0.1.1
Server version: v0.1.0
$ minikube stop > /dev/null
$ bin/go-run cli version
github.com/runconduit/conduit/cli
Client version: v0.1.1
Server version: unavailable
The difference, I believe, is that err here means "could not instantiate kubectl", not "could not get version from server". The logic that handles errors from the API was moved here: https://github.com/runconduit/conduit/pull/86/files/c49c7e738bef442dbaba164f812c85a25b8d3144#diff-b784f3ae92cb061761114526d43f2e33R58 and is tested here: https://github.com/runconduit/conduit/pull/86/files/c49c7e738bef442dbaba164f812c85a25b8d3144#diff-edf10645fffdcf276967004f4a31ac4fR29
| return nil, err | ||
| } | ||
|
|
||
| func newApiClient(kubeApi k8s.KubernetesApi) (pb.ApiClient, error) { |
There was a problem hiding this comment.
curious: was this change to newApiClient needed for this round of test changes, or is it in anticipation of future testing? how does this change relate to mockApiClient?
There was a problem hiding this comment.
I was trying to be consistent with the way we create other objects. With this PR we have every command instantiating the objects it needs and wiring them together. Arguably this function should not be in the root command but rather follow the pattern established elsewhere and be something like conduitclient.MakeApiClient(kubeApi k8s.KubernetesApi) (pb.ApiClient, error), but I didn't want to propose something like this before there was an actual need for it.
| var versionCmd = &cobra.Command{ | ||
| Use: "version", | ||
| Short: "Print the client and server version information", | ||
| Long: "Print the client and server version information.", | ||
| Args: cobra.NoArgs, | ||
| Run: exitSilentlyOnError(func(cmd *cobra.Command, args []string) error { | ||
| fmt.Println("Client version: " + controller.Version) |
There was a problem hiding this comment.
I'm running this from a Docker container, perhaps that's why I see no output?
If either k8s.MakeK8sAPi() or newApiClient() fail, it prints nothing. I think we should print the client version no matter what, and then perhaps let the user know what went wrong.
pcalcado
force-pushed
the
phil/49-conduit-when-pod-doesnt-exist
branch
9 times, most recently
from
e3b0859 to
64e30a5
Compare
|
@siggy I can't reproduce the UX issue your reported on #86 (review) , but I have the impression that this commit solved the problem: 64e30a5 could you test? Also, I have no idea why this says the DCO isn't there? |
Signed-off-by: Phil Calcado <phil@buoyant.io>
Signed-off-by: Phil Calcado <phil@buoyant.io>
Signed-off-by: Phil Calcado <phil@buoyant.io>
Signed-off-by: Phil Calcado <phil@buoyant.io>
Signed-off-by: Phil Calcado <phil@buoyant.io>
Signed-off-by: Phil Calcado <phil@buoyant.io>
Signed-off-by: Phil Calcado <phil@buoyant.io>
pcalcado
force-pushed
the
phil/49-conduit-when-pod-doesnt-exist
branch
from
64e30a5 to
2e2598b
Compare
There was a problem hiding this comment.
you are correct this was never reproducible with your branch. i had forgotten to set DOCKER_FORCE_BUILD and had a 3-week old conduit binary in my docker image. apologies for the delay here.
i've left one comment re: double printing of error messages. not a big deal. it would also be fine to roll back to your original implementation since that worked anyway.
cli/cmd/root.go
Outdated
| return func(cmd *cobra.Command, args []string) { | ||
| if err := f(cmd, args); err != nil { | ||
| os.Exit(2) // Reserve 1 for usage errors. | ||
| fmt.Fprintln(os.Stderr, err) |
There was a problem hiding this comment.
this causes errors to be printed twice:
$ conduit version --kubeconfig foo
error instantiating Kubernetes API client: stat foo: no such file or directory
Client version: v0.1.1
Server version: unavailable
error instantiating Kubernetes API client: stat foo: no such file or directory| } | ||
| }) | ||
|
|
||
| t.Run("Returns empty list if no [ods found", func(t *testing.T) { |
|
@siggy awesome, thanks! I will remove the last commit and later start a conversation around how we do output in the CLI so that we have a standard way to print |
pcalcado
force-pushed
the
phil/49-conduit-when-pod-doesnt-exist
branch
from
2e2598b to
da97dc6
Compare
The `control::destination` exposes an important trait, `Bind`, that abstracts the logic of instantiating a new service for an individual endpoint (i.e., in a load balancer). This interface is not specific to our service discovery implementation, and can easily be used to model other types of client factory. In the spirit of consolidating our HTTP-specific logic, and making the core APIs of the proxy more visible, this change renames the `Bind` trait to `NewClient`, simplifies the trait to have fewer type parameters, and documents this new generalized API.
As I work on the diagnosis CLI experience, I had to go through our current CLI commands and understand them better. I've written tests and refactored things a bit to document this understanding.