-
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
rsh support for multiple resources #8279
rsh support for multiple resources #8279
Conversation
} | ||
|
||
switch resourceType { | ||
case "pods": |
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.
add a no-op comment here so it doesn't look like a fallthrough was intended
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.
ok
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, return name, nil
to keep the intent close to the check, and remove the return at the end of the func
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.
done
[test] |
} | ||
pod := &pods.Items[0] | ||
return pod, len(pods.Items), nil | ||
return &pods.Items[0], len(pods.Items), 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.
if there are multiple pods, shouldn't this prefer running ones to non-running, ready to non-ready, etc?
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 was filtering active pods and then sorting to the oldest but then remembered that @deads2k was against it when he was adding this upstream.
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 was filtering active pods and then sorting to the oldest but then remembered that @deads2k was against it when he was adding this upstream.
I want different sorts for different uses:
rsh
: pod better be running, but it probably doesn't matter which one.logs
: show me the crashlooping one, because all happy families are the same.attach
: I probably want the most recent pod.
I haven't looked at this pull, but does it allow different comparators?
This is ready for another pass. I still need to add unit tests for GetFirstPod cc also @soltysh |
@@ -389,7 +392,8 @@ func NewFactory(optionalClientConfig clientcmd.ClientConfig) *Factory { | |||
return nil, errors.New("provided options object is not a PodLogOptions") | |||
} | |||
selector := labels.SelectorFromSet(t.Spec.Selector) | |||
pod, numPods, err := GetFirstPod(c, t.Namespace, selector) | |||
f := func(pods []*api.Pod) sort.Interface { return controller.ActivePods(pods) } |
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.
can't you just pass in controller.ActivePods
?
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.
Nope, I cannot use the interface argument as an expression.
Rebase |
Can you add a test for pod/ as well. |
done |
Add support for replication controllers, deployment configs, jobs, and daemon sets.
Evaluated for origin test up to 2a1d195 |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/3505/) |
Any other comments? |
LGTM [merge] |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/5859/) (Image: devenv-rhel7_4149) |
Evaluated for origin merge up to 2a1d195 |
Fixes #6344
Fixes #8247
@smarterclayton @fabianofranz @deads2k PTAL