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.

Sign up for GitHub

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

Jump to bottom

Close the stdin/tty when using podman as a restAPI. #8717

Merged
merged 1 commit into from
Dec 17, 2020
Merged

Close the stdin/tty when using podman as a restAPI. #8717

merged 1 commit into from
Dec 17, 2020

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Dec 14, 2020

Currently the service is attempting to prompt on shortname expansion if you run
with a terminal. This change will cause the service to default to no terminal
and not prompt.

Signed-off-by: Daniel J Walsh [email protected]

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 14, 2020
@rhatdan
Copy link
Member Author

rhatdan commented Dec 14, 2020

Helps fix: #8700

@rhatdan
Copy link
Member Author

rhatdan commented Dec 14, 2020

@mtrmac @vrothberg PTAL

@mheon
Copy link
Member

mheon commented Dec 14, 2020

LGTM

Luap99
Luap99 reviewed Dec 14, 2020
View reviewed changes
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

The code does not compile but that is definitely what I want.

cmd/podman/system/service.go Outdated
@@ -53,6 +54,9 @@ func init() {

timeFlagName := "time"
flags.Int64VarP(&srvArgs.Timeout, timeFlagName, "t", 5, "Time until the service session expires in seconds. Use 0 to disable the timeout")
ttyFlagName := "tty"
flags.BoolVar(&srvArgs.TTY, ttyFlagName, false, "Allow stdin to interact with the service")
_ = srvCmd.RegisterFlagCompletionFunc(ttyFlagName, completion.AutocompleteNone)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
_ = srvCmd.RegisterFlagCompletionFunc(ttyFlagName, completion.AutocompleteNone)

Bool flags should not have this.

vrothberg
vrothberg reviewed Dec 14, 2020
View reviewed changes
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

I would be cool to close stdin unconditionally.

Thanks for tackling it, @rhatdan and thanks to @mtrmac for the great idea.

mtrmac
mtrmac requested changes Dec 14, 2020
View reviewed changes
cmd/podman/system/service_abi.go Outdated Show resolved Hide resolved
cmd/podman/system/service_abi.go Outdated Show resolved Hide resolved
@rhatdan rhatdan force-pushed the stdin branch 2 times, most recently from b84a719 to 0310a5f Compare December 15, 2020 10:10
vrothberg
vrothberg reviewed Dec 15, 2020
View reviewed changes
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM

mtrmac
mtrmac reviewed Dec 15, 2020
View reviewed changes
cmd/podman/system/service_abi.go Show resolved Hide resolved
cmd/podman/system/service_abi.go Outdated Show resolved Hide resolved
cmd/podman/system/service_abi.go Outdated Show resolved Hide resolved
@rhatdan
Copy link
Member Author

rhatdan commented Dec 15, 2020

@mtrmac Ok, now I am using unix.Dup2 and looking at the process fds,
ls -l /proc/373817/fd/0
lr-x------. 1 dwalsh dwalsh 64 Dec 15 10:42 /proc/373817/fd/0 -> /dev/null

mtrmac
mtrmac reviewed Dec 15, 2020
View reviewed changes
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

LGTM. (I’m not going to worry about flushing buffers…)

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mtrmac, rhatdan

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mheon
Copy link
Member

mheon commented Dec 15, 2020

LGTM

@TomSweeneyRedHat
Copy link
Member

LGTM
but EXT. services test is failing which is new to me. I don't think it's related to the change, but I'm not sure.

@edsantiago
Copy link
Member

Ext. services: you'll need to rebase, sorry. See #8726 (comment) for context

@rhatdan rhatdan force-pushed the stdin branch 2 times, most recently from e43225f to c7125c2 Compare December 16, 2020 20:10
@rhatdan rhatdan added the lgtm Indicates that a PR is ready to be merged. label Dec 16, 2020
@rhatdan
Copy link
Member Author

rhatdan commented Dec 16, 2020

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 16, 2020
@rhatdan
Close the stdin/tty when using podman as a restAPI.
b96c348 
Currently the service is attempting to prompt on shortname expansion if you run
with a terminal. This change will cause the service to default to no terminal
and not prompt.

Signed-off-by: Daniel J Walsh <[email protected]>
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 17, 2020
@openshift-ci-robot
Copy link
Collaborator

New changes are detected. LGTM label has been removed.

@rhatdan rhatdan added the lgtm Indicates that a PR is ready to be merged. label Dec 17, 2020
@rhatdan
Copy link
Member Author

rhatdan commented Dec 17, 2020

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 17, 2020
@openshift-merge-robot openshift-merge-robot merged commit 7592f8f into containers:master Dec 17, 2020
@vrothberg vrothberg mentioned this pull request Dec 18, 2020
Closed
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Luap99 Luap99 Luap99 left review comments

@vrothberg vrothberg vrothberg left review comments

@mtrmac mtrmac mtrmac approved these changes

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@rhatdan @mheon @openshift-ci-robot @TomSweeneyRedHat @edsantiago @mtrmac @vrothberg @Luap99 @openshift-merge-robot