Exec should be easier to reuse as a command #11729
Conversation
|
GCE e2e build/test passed for commit eb05fb97ab3e12bf59c637cfae2c65c4674c8c61. |
smarterclayton
force-pushed
the
make_exec_flexible
branch
from
eb05fb9 to
af1fac4
Compare
|
GCE e2e build/test passed for commit af1fac435bfca656b57be47633ed1ba594efbba3. |
| Err io.Writer | ||
|
|
||
| Executor RemoteExecutor | ||
| Client *client.Client |
There was a problem hiding this comment.
Which interface do you actually need?
There was a problem hiding this comment.
Several, and we don't have an interface for rest client in client today
(just in resource). Future feature.
On Jul 23, 2015, at 7:37 AM, David Eads notifications@github.com wrote:
In pkg/kubectl/cmd/exec.go
#11729 (comment)
:
- tty bool
+// ExecOptions declare the arguments accepted by the Exec command
+type ExecOptions struct {- Namespace string
- PodName string
- ContainerName string
- Stdin bool
- TTY bool
- Args []string
+- In io.Reader
- Out io.Writer
- Err io.Writer
+- Executor RemoteExecutor
- Client *client.Client
Which interface do you actually need?
—
Reply to this email directly or view it on GitHub
https://github.com/GoogleCloudPlatform/kubernetes/pull/11729/files#r35312477
.
|
Minor comments. Big improvement. |
|
Will have something soon. On Jul 23, 2015, at 7:50 AM, David Eads notifications@github.com wrote: Minor comments. Big improvement. — |
|
cc @bgrant0607 |
smarterclayton
force-pushed
the
make_exec_flexible
branch
from
af1fac4 to
a40bdaf
Compare
|
Comments addressed On Thu, Jul 23, 2015 at 2:20 PM, Vish Kannan notifications@github.com
Clayton Coleman | Lead Engineer, OpenShift |
| if len(argsIn) < 1 { | ||
| return "", "", nil, cmdutil.UsageError(cmd, "COMMAND is required for exec") | ||
| return cmdutil.UsageError(cmd, "COMMAND is required for exec") |
There was a problem hiding this comment.
seems like validation best left to Validate
There was a problem hiding this comment.
Double validates because this is specific to command behavior (Complete)
not usage behavior (Validate)
On Thu, Jul 23, 2015 at 3:43 PM, David Eads notifications@github.com
wrote:
In pkg/kubectl/cmd/exec.go
#11729 (comment)
:if len(argsIn) < 1 {
return "", "", nil, cmdutil.UsageError(cmd, "COMMAND is required for exec")return cmdutil.UsageError(cmd, "COMMAND is required for exec")seems like validation best left to Validate
—
Reply to this email directly or view it on GitHub
https://github.com/GoogleCloudPlatform/kubernetes/pull/11729/files#r35362768
.
Clayton Coleman | Lead Engineer, OpenShift
|
Just aesthetic disagreements. lgtm. |
|
GCE e2e build/test failed for commit a40bdafa12a1c62e6ed13066edd5ef5c37ef1396. |
|
ok to test |
|
I support refactoring all commands for use as libraries: #7311. |
|
GCE e2e build/test passed for commit a40bdafa12a1c62e6ed13066edd5ef5c37ef1396. |
|
I think this is ready for merge unless there are more comments? |
|
Feel free to put the lgtm label on so that oncall will merge. |
|
LGTM On Fri, Jul 24, 2015 at 12:15 PM, Brian Grant notifications@github.com
|
bgrant0607
added
the
lgtm
|
I meant github label. I just tagged it. |
|
I know - that was more for Clayton since he asked if there were any other
|
|
@smarterclayton please rebase to fix shippable |
We have a few use cases where we want to have shortcut commands for common behavior. In these cases, we want to default commands with a simpler syntax, so being able to reuse Exec from code but not share the same arguments is useful. In this case, we're introducing 'rsh' which tries to run exec -itp -- bash.
smarterclayton
force-pushed
the
make_exec_flexible
branch
from
a40bdaf to
0b5410f
Compare
|
GCE e2e build/test passed for commit 0b5410f. |
Exec should be easier to reuse as a command
We have a few use cases where we want to have shortcut commands
for common behavior. In these cases, we want to default commands
with a simpler syntax, so being able to reuse Exec from code but
not share the same arguments is useful.
In this case, we're introducing 'rsh' which tries to run exec -itp --
bash.
@ncdc / @deads2k