Skip to content
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

Update Exec and Portforward client to use pod subresource #7715

Merged
merged 2 commits into from May 7, 2015

Conversation

csrwng
Copy link
Contributor

@csrwng csrwng commented May 4, 2015

Switches cli cmd for portfoward and exec to use /pods/[name]/exec and /pod/[name]/portforward

@csrwng
Copy link
Contributor Author

csrwng commented May 4, 2015

@smarterclayton ptal

doStdin := (e.stdin != nil)
doStdout := (e.stdout != nil)
doStderr := (!e.tty && e.stderr != nil)

if doStdin {
e.req.Param(api.ExecStdinParam, "1")
e.req.Param("stdin", "true")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where'd these constants go? Deleted? If so, you should be creating an api.ExecOptions and then converting it to the desired API version, then converting it to a map.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smarterclayton they weren't deleted, just not applicable anymore. They were for the internal endpoint. I will use the api.ExecOptions object to do the conversion.

@csrwng
Copy link
Contributor Author

csrwng commented May 6, 2015

Updated parameter conversion for exec

@csrwng
Copy link
Contributor Author

csrwng commented May 6, 2015

Moved conversion code under pkg/conversion

func RunPortForward(f *cmdutil.Factory, cmd *cobra.Command, args []string) error {
podName := cmdutil.GetFlagString(cmd, "pod")
if len(podName) == 0 {
type portForwardCmd struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this abstraction? Seems overly complex, not sure what you're trying to do here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only reason is to test... abstract out call to portforward.New(...) and following call to that object's ForwardPorts method. I'll think of a way to simplify it.

@csrwng
Copy link
Contributor Author

csrwng commented May 7, 2015

complexity reduced

return fw.ForwardPorts()
}

func runPortForward(f *cmdutil.Factory, cmd *cobra.Command, args []string, fw portForwarder) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this no longer public?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't need to be. It's only used by the command.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our pattern is that all of these are public.

csrwng added 2 commits May 7, 2015 14:25
JSON struct tags are used as parameter names, fields that do not have
the omitempty marker are always included.
@csrwng
Copy link
Contributor Author

csrwng commented May 7, 2015

reverted to public functions

@smarterclayton
Copy link
Contributor

LGTM

smarterclayton added a commit that referenced this pull request May 7, 2015
Update Exec and Portforward client to use pod subresource
@smarterclayton smarterclayton merged commit f2b6c41 into kubernetes:master May 7, 2015
@csrwng csrwng deleted the exec_portfw_client branch July 28, 2015 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants