-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
switch logs, idle commands to externals #20343
switch logs, idle commands to externals #20343
Conversation
60f9c87
to
2d5e0e4
Compare
pkg/oc/cli/logs/logs.go
Outdated
appsapi.Resource("deploymentconfigs"): | ||
case appsapiv1.Resource("deploymentconfig"), | ||
appsapiv1.Resource("deploymentconfigs"): | ||
// TODO: switch to using appsapiv1.DeploymentLogOptions once originpolymorphichelpers.LogsForObjectFn supports appsv1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deads2k fyi. Using the external api here to create the DeploymentLogOptions object would also involve changes to the logsforobject Origin polymorphic helper: https://github.com/openshift/origin/blob/master/pkg/oc/originpolymorphichelpers/logsforobject.go#L33
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deads2k fyi. Using the external api here to create the DeploymentLogOptions object would also involve changes to the logsforobject Origin polymorphic helper:
update that in this pull..
/test extended_clusterup |
2d5e0e4
to
0733f44
Compare
/test gcp |
f2dc629
to
5c4ee6b
Compare
if err != nil { | ||
return nil, err | ||
} | ||
return appsmanualclient.NewRolloutLogClient(appsClient.Apps().RESTClient(), t.Namespace).Logs(t.Name, *dopts), nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deads2k not sure how to handle this - do we have a rollout client (or buildlogClient for the case below) whose Logs method receives versioned options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i made an external version for this here #20362
8240000
to
16898e0
Compare
16898e0
to
fe66212
Compare
fe66212
to
ee05326
Compare
ee05326
to
7eb89e6
Compare
@DirectXMan12 could you give a quick look to the unidling_controller changes? Should be just the second commit |
unidling changes look fine. |
7eb89e6
to
a9a649a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to be strict about the import aliases so that they are self-explanatory. Fix them and rebase, please.
pkg/oc/cli/logs/logs_test.go
Outdated
@@ -12,9 +12,10 @@ import ( | |||
kcmd "k8s.io/kubernetes/pkg/kubectl/cmd" | |||
"k8s.io/kubernetes/pkg/kubectl/genericclioptions" | |||
|
|||
buildapi "github.com/openshift/api/build/v1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
buildv1
always
pkg/oc/cli/logs/logs_test.go
Outdated
appsapi "github.com/openshift/origin/pkg/apps/apis/apps" | ||
buildapi "github.com/openshift/origin/pkg/build/apis/build" | ||
buildfake "github.com/openshift/origin/pkg/build/generated/internalclientset/fake" | ||
internalbuildapi "github.com/openshift/origin/pkg/build/apis/build" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
buildapi
always
Object: tc.o, | ||
Namespace: "foo", | ||
}, | ||
Client: fakebc.Build(), | ||
} | ||
if err := o.RunLog(); err != nil { | ||
if err := o.runLogPipeline(); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test TestRunLogForPipelineStrategy
was only testing that portion of the code. Splitting it out made it easier to test
pkg/oc/cli/logs/logs.go
Outdated
"github.com/openshift/api/apps" | ||
"github.com/openshift/api/build" | ||
appsapi "github.com/openshift/origin/pkg/apps/apis/apps" | ||
appsapiv1 "github.com/openshift/api/apps/v1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
appsv1
pkg/oc/cli/logs/logs.go
Outdated
appsapi "github.com/openshift/origin/pkg/apps/apis/apps" | ||
appsapiv1 "github.com/openshift/api/apps/v1" | ||
buildapiv1 "github.com/openshift/api/build/v1" | ||
buildclientv1 "github.com/openshift/client-go/build/clientset/versioned/typed/build/v1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
buildv1client
@@ -8,6 +8,7 @@ import ( | |||
appsclient "github.com/openshift/origin/pkg/apps/generated/internalclientset/typed/apps/internalversion" | |||
"github.com/openshift/origin/pkg/cmd/server/bootstrappolicy" | |||
unidlingcontroller "github.com/openshift/origin/pkg/unidling/controller" | |||
kubernetes "k8s.io/client-go/kubernetes/typed/core/v1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
corev1
pkg/oc/cli/idle/idle.go
Outdated
extensions "k8s.io/api/extensions/v1beta1" | ||
"k8s.io/apimachinery/pkg/api/meta" | ||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
"k8s.io/apimachinery/pkg/runtime" | ||
"k8s.io/apimachinery/pkg/runtime/schema" | ||
"k8s.io/apimachinery/pkg/types" | ||
"k8s.io/apimachinery/pkg/util/strategicpatch" | ||
"k8s.io/client-go/kubernetes" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clientset
@@ -20,6 +20,7 @@ import ( | |||
"k8s.io/apimachinery/pkg/runtime" | |||
"k8s.io/apimachinery/pkg/types" | |||
utilruntime "k8s.io/apimachinery/pkg/util/runtime" | |||
kubernetes "k8s.io/client-go/kubernetes/typed/core/v1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
corev1client
pkg/unidling/util/scale.go
Outdated
kextapi "k8s.io/api/extensions/v1beta1" | ||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
"k8s.io/apimachinery/pkg/runtime" | ||
"k8s.io/apimachinery/pkg/types" | ||
"k8s.io/apimachinery/pkg/util/strategicpatch" | ||
kubernetes "k8s.io/client-go/kubernetes/typed/core/v1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
corev1client
return nil, err | ||
} | ||
return appsmanualclientv1.NewRolloutLogClient(appsClient.RESTClient(), t.Namespace).Logs(t.Name, *dopts), nil | ||
case *buildapiv1.Build: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're missing buildv1.BuildConfig
as well.
if err != nil { | ||
return nil, err | ||
} | ||
return buildmanualclientv1.NewBuildLogClient(buildClient.RESTClient(), t.Namespace).Logs(t.Name, *bopts), nil | ||
case *buildapi.Build: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, since we've just migrated logs to external, do we need internals here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I take that back, I see we're using this in quite a few places so we need to provide a smooth path. Sighs....
53905d1
to
c44f49e
Compare
6ef5eba
to
2c27df7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few nits, fix them a feel free to lgtm
/approve
pkg/oc/cli/logs/logs.go
Outdated
"github.com/openshift/api/build" | ||
appsapi "github.com/openshift/origin/pkg/apps/apis/apps" | ||
appsv1 "github.com/openshift/api/apps/v1" | ||
buildapiv1 "github.com/openshift/api/build/v1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
buildv1
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: juanvallejo, soltysh The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
399a79d
to
ee1036b
Compare
/test extended_clusterup |
/test extended_builds |
ee1036b
to
62c5374
Compare
New changes are detected. LGTM label has been removed. |
cc @deads2k
Depends on #20362
Switches more commands to deal with external versions of objects
Picks kubernetes/kubernetes#66352