From 9fe3318470e94d33920d7f8abe1225763d95826e Mon Sep 17 00:00:00 2001 From: juanvallejo Date: Mon, 30 Jul 2018 17:58:14 -0400 Subject: [PATCH] update log cmd to use corev1.PodLogOptions --- pkg/oc/cli/logs/logs.go | 10 +- .../kubernetes/pkg/kubectl/cmd/logs_test.go | 270 +++++++++++------- 2 files changed, 168 insertions(+), 112 deletions(-) diff --git a/pkg/oc/cli/logs/logs.go b/pkg/oc/cli/logs/logs.go index e7592258d4ec..5c262f19f84d 100644 --- a/pkg/oc/cli/logs/logs.go +++ b/pkg/oc/cli/logs/logs.go @@ -6,9 +6,9 @@ import ( "github.com/spf13/cobra" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - kapi "k8s.io/kubernetes/pkg/apis/core" kcmd "k8s.io/kubernetes/pkg/kubectl/cmd" "k8s.io/kubernetes/pkg/kubectl/cmd/templates" kcmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util" @@ -99,7 +99,7 @@ func NewCmdLogs(name, baseName string, f kcmdutil.Factory, streams genericcliopt cmd.SuggestFor = []string{"builds", "deployments"} cmd.Run = func(cmd *cobra.Command, args []string) { kcmdutil.CheckErr(o.Complete(f, cmd, args)) - kcmdutil.CheckErr(o.Validate()) + kcmdutil.CheckErr(o.Validate(args)) kcmdutil.CheckErr(o.RunLog()) } @@ -151,8 +151,8 @@ func (o *LogsOptions) Complete(f kcmdutil.Factory, cmd *cobra.Command, args []st // Validate runs the upstream validation for the logs command and then it // will validate any OpenShift-specific log options. -func (o *LogsOptions) Validate() error { - if err := o.KubeLogOptions.Validate(); err != nil { +func (o *LogsOptions) Validate(args []string) error { + if err := o.KubeLogOptions.Validate(args); err != nil { return err } if o.Options == nil { @@ -176,7 +176,7 @@ func (o *LogsOptions) Validate() error { // RunLog will run the upstream logs command and may use an OpenShift // logOptions object. func (o *LogsOptions) RunLog() error { - podLogOptions := o.KubeLogOptions.Options.(*kapi.PodLogOptions) + podLogOptions := o.KubeLogOptions.Options.(*corev1.PodLogOptions) infos, err := o.Builder(). WithScheme(scheme.Scheme, scheme.Scheme.PrioritizedVersionsAllGroups()...). NamespaceParam(o.Namespace).DefaultNamespace(). diff --git a/vendor/k8s.io/kubernetes/pkg/kubectl/cmd/logs_test.go b/vendor/k8s.io/kubernetes/pkg/kubectl/cmd/logs_test.go index 7b82432532e4..ad172b63e498 100644 --- a/vendor/k8s.io/kubernetes/pkg/kubectl/cmd/logs_test.go +++ b/vendor/k8s.io/kubernetes/pkg/kubectl/cmd/logs_test.go @@ -17,29 +17,20 @@ limitations under the License. package cmd import ( - "bytes" "errors" - "io/ioutil" - "net/http" + "fmt" + "io" "strings" "testing" "time" - "github.com/spf13/cobra" - - "fmt" - + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" restclient "k8s.io/client-go/rest" - "k8s.io/client-go/rest/fake" - "k8s.io/kubernetes/pkg/api/legacyscheme" api "k8s.io/kubernetes/pkg/apis/core" - "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset" cmdtesting "k8s.io/kubernetes/pkg/kubectl/cmd/testing" "k8s.io/kubernetes/pkg/kubectl/genericclioptions" - "k8s.io/kubernetes/pkg/kubectl/polymorphichelpers" - "k8s.io/kubernetes/pkg/kubectl/scheme" ) func TestLog(t *testing.T) { @@ -48,11 +39,8 @@ func TestLog(t *testing.T) { pod *api.Pod }{ { - name: "v1 - pod log", - version: "v1", - podPath: "/namespaces/test/pods/foo", - logPath: "/api/v1/namespaces/test/pods/foo/log", - pod: testPod(), + name: "v1 - pod log", + pod: testPod(), }, } for _, test := range tests { @@ -61,41 +49,19 @@ func TestLog(t *testing.T) { tf := cmdtesting.NewTestFactory().WithNamespace("test") defer tf.Cleanup() - codec := legacyscheme.Codecs.LegacyCodec(scheme.Scheme.PrioritizedVersionsAllGroups()...) - ns := legacyscheme.Codecs - - tf.Client = &fake.RESTClient{ - NegotiatedSerializer: ns, - Client: fake.CreateHTTPClient(func(req *http.Request) (*http.Response, error) { - switch p, m := req.URL.Path, req.Method; { - case p == test.podPath && m == "GET": - body := objBody(codec, test.pod) - return &http.Response{StatusCode: 200, Header: defaultHeader(), Body: body}, nil - case p == test.logPath && m == "GET": - body := ioutil.NopCloser(bytes.NewBufferString(logContent)) - return &http.Response{StatusCode: 200, Header: defaultHeader(), Body: body}, nil - default: - t.Errorf("%s: unexpected request: %#v\n%#v", test.name, req.URL, req) - return nil, nil - } - }), - } - tf.ClientConfigVal = defaultClientConfig() - oldLogFn := polymorphichelpers.LogsForObjectFn - defer func() { - polymorphichelpers.LogsForObjectFn = oldLogFn - }() - clientset, err := tf.ClientSet() - if err != nil { - t.Fatal(err) - } - polymorphichelpers.LogsForObjectFn = logTestMock{client: clientset}.logsForObject - streams, _, buf, _ := genericclioptions.NewTestIOStreams() - cmd := NewCmdLogs(tf, streams) - cmd.Flags().Set("namespace", "test") - cmd.Run(cmd, []string{"foo"}) + mock := &logTestMock{ + logsContent: logContent, + } + + opts := NewLogsOptions(streams, false) + opts.Namespace = "test" + opts.Object = test.pod + opts.Options = &corev1.PodLogOptions{} + opts.LogsForObject = mock.mockLogsForObject + opts.ConsumeRequestFn = mock.mockConsumeRequest + opts.RunLogs() if buf.String() != logContent { t.Errorf("%s: did not get expected log content. Got: %s", test.name, buf.String()) @@ -119,66 +85,150 @@ func testPod() *api.Pod { } } -func TestValidateLogFlags(t *testing.T) { +func TestValidateLogOptions(t *testing.T) { f := cmdtesting.NewTestFactory() defer f.Cleanup() f.WithNamespace("") tests := []struct { name string - flags map[string]string args []string + opts func(genericclioptions.IOStreams) *LogsOptions expected string }{ { - name: "since & since-time", - flags: map[string]string{"since": "1h", "since-time": "2006-01-02T15:04:05Z"}, + name: "since & since-time", + opts: func(streams genericclioptions.IOStreams) *LogsOptions { + o := NewLogsOptions(streams, false) + o.SinceSeconds = time.Hour + o.SinceTime = "2006-01-02T15:04:05Z" + + var err error + o.Options, err = o.ToLogOptions() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + return o + }, args: []string{"foo"}, expected: "at most one of `sinceTime` or `sinceSeconds` may be specified", }, { - name: "negative since-time", - flags: map[string]string{"since": "-1s"}, + name: "negative since-time", + opts: func(streams genericclioptions.IOStreams) *LogsOptions { + o := NewLogsOptions(streams, false) + o.SinceSeconds = -1 * time.Second + + var err error + o.Options, err = o.ToLogOptions() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + return o + }, args: []string{"foo"}, expected: "must be greater than 0", }, { - name: "negative limit-bytes", - flags: map[string]string{"limit-bytes": "-100"}, + name: "negative limit-bytes", + opts: func(streams genericclioptions.IOStreams) *LogsOptions { + o := NewLogsOptions(streams, false) + o.LimitBytes = -100 + + var err error + o.Options, err = o.ToLogOptions() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + return o + }, args: []string{"foo"}, expected: "must be greater than 0", }, { - name: "negative tail", - flags: map[string]string{"tail": "-100"}, + name: "negative tail", + opts: func(streams genericclioptions.IOStreams) *LogsOptions { + o := NewLogsOptions(streams, false) + o.Tail = -100 + + var err error + o.Options, err = o.ToLogOptions() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + return o + }, args: []string{"foo"}, expected: "must be greater than or equal to 0", }, { - name: "container name combined with --all-containers", - flags: map[string]string{"all-containers": "true"}, + name: "container name combined with --all-containers", + opts: func(streams genericclioptions.IOStreams) *LogsOptions { + o := NewLogsOptions(streams, true) + o.Container = "my-container" + + var err error + o.Options, err = o.ToLogOptions() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + return o + }, args: []string{"my-pod", "my-container"}, expected: "--all-containers=true should not be specified with container", }, + { + name: "container name combined with second argument", + opts: func(streams genericclioptions.IOStreams) *LogsOptions { + o := NewLogsOptions(streams, false) + o.Container = "my-container" + o.ContainerNameSpecified = true + + var err error + o.Options, err = o.ToLogOptions() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + return o + }, + args: []string{"my-pod", "my-container"}, + expected: "only one of -c or an inline", + }, + { + name: "follow and selector conflict", + opts: func(streams genericclioptions.IOStreams) *LogsOptions { + o := NewLogsOptions(streams, false) + o.Selector = "foo" + o.Follow = true + + var err error + o.Options, err = o.ToLogOptions() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + return o + }, + expected: "only one of follow (-f) or selector (-l) is allowed", + }, } for _, test := range tests { streams := genericclioptions.NewTestIOStreamsDiscard() - cmd := NewCmdLogs(f, streams) - out := "" - for flag, value := range test.flags { - cmd.Flags().Set(flag, value) - } - // checkErr breaks tests in case of errors, plus we just - // need to check errors returned by the command validation - o := NewLogsOptions(streams, test.flags["all-containers"] == "true") - cmd.Run = func(cmd *cobra.Command, args []string) { - o.Complete(f, cmd, args) - out = o.Validate().Error() + + o := test.opts(streams) + err := o.Validate(test.args) + if err == nil { + t.Fatalf("expected error %q, got none", test.expected) } - cmd.Run(cmd, test.args) - if !strings.Contains(out, test.expected) { - t.Errorf("%s: expected to find:\n\t%s\nfound:\n\t%s\n", test.name, test.expected, out) + if !strings.Contains(err.Error(), test.expected) { + t.Errorf("%s: expected to find:\n\t%s\nfound:\n\t%s\n", test.name, test.expected, err.Error()) } } } @@ -190,49 +240,49 @@ func TestLogComplete(t *testing.T) { tests := []struct { name string args []string - flags map[string]string + opts func(genericclioptions.IOStreams) *LogsOptions expected string }{ { - name: "No args case", - flags: map[string]string{"selector": ""}, + name: "No args case", + opts: func(streams genericclioptions.IOStreams) *LogsOptions { + return NewLogsOptions(streams, false) + }, expected: "'logs (POD | TYPE/NAME) [CONTAINER_NAME]'.\nPOD or TYPE/NAME is a required argument for the logs command", }, { - name: "One args case", - args: []string{"foo"}, - flags: map[string]string{"selector": "foo"}, + name: "One args case", + args: []string{"foo"}, + opts: func(streams genericclioptions.IOStreams) *LogsOptions { + o := NewLogsOptions(streams, false) + o.Selector = "foo" + return o + }, expected: "only a selector (-l) or a POD name is allowed", }, { - name: "Two args case", - args: []string{"foo", "foo1"}, - flags: map[string]string{"container": "foo1"}, - expected: "only one of -c or an inline [CONTAINER] arg is allowed", - }, - { - name: "More than two args case", - args: []string{"foo", "foo1", "foo2"}, - flags: map[string]string{"tail": "1"}, + name: "More than two args case", + args: []string{"foo", "foo1", "foo2"}, + opts: func(streams genericclioptions.IOStreams) *LogsOptions { + o := NewLogsOptions(streams, false) + o.Tail = 1 + return o + }, expected: "'logs (POD | TYPE/NAME) [CONTAINER_NAME]'.\nPOD or TYPE/NAME is a required argument for the logs command", }, - { - name: "follow and selecter conflict", - flags: map[string]string{"selector": "foo", "follow": "true"}, - expected: "only one of follow (-f) or selector (-l) is allowed", - }, } for _, test := range tests { cmd := NewCmdLogs(f, genericclioptions.NewTestIOStreamsDiscard()) - var err error out := "" - for flag, value := range test.flags { - cmd.Flags().Set(flag, value) - } + // checkErr breaks tests in case of errors, plus we just // need to check errors returned by the command validation - o := NewLogsOptions(genericclioptions.NewTestIOStreamsDiscard(), false) - err = o.Complete(f, cmd, test.args) + o := test.opts(genericclioptions.NewTestIOStreamsDiscard()) + err := o.Complete(f, cmd, test.args) + if err == nil { + t.Fatalf("expected error %q, got none", test.expected) + } + out = err.Error() if !strings.Contains(out, test.expected) { t.Errorf("%s: expected to find:\n\t%s\nfound:\n\t%s\n", test.name, test.expected, out) @@ -241,17 +291,23 @@ func TestLogComplete(t *testing.T) { } type logTestMock struct { - client internalclientset.Interface + logsContent string } -func (m logTestMock) logsForObject(restClientGetter genericclioptions.RESTClientGetter, object, options runtime.Object, timeout time.Duration) (*restclient.Request, error) { - switch t := object.(type) { +func (l *logTestMock) mockConsumeRequest(req *restclient.Request, out io.Writer) error { + fmt.Fprintf(out, l.logsContent) + return nil +} + +func (l *logTestMock) mockLogsForObject(restClientGetter genericclioptions.RESTClientGetter, object, options runtime.Object, timeout time.Duration) (*restclient.Request, error) { + switch object.(type) { case *api.Pod: - opts, ok := options.(*api.PodLogOptions) + _, ok := options.(*corev1.PodLogOptions) if !ok { return nil, errors.New("provided options object is not a PodLogOptions") } - return m.client.Core().Pods(t.Namespace).GetLogs(t.Name, opts), nil + + return &restclient.Request{}, nil default: return nil, fmt.Errorf("cannot get the logs from %T", object) }