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

Bugfix #1343358: Enforce --tty=false flag for "oc debug" #9637

Merged
merged 1 commit into from
Jul 26, 2016

Conversation

damemi
Copy link
Contributor

@damemi damemi commented Jun 29, 2016

Changes behavior for "oc debug" command, specifically with --tty flag. Prior behavior left --tty essentially useless, as it was not checked if the user specifically requested to disable TTY by passing --tty=false (if the command was run in a terminal, and --no-tty was not set to true, it would always default to a TTY, even with --tty=false set.

This change allows both flags to remain for compliance with any other programs that might use them, at least until one is deprecated as it has been suggested that having both is redundant.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1343358

@ncdc
Copy link
Contributor

ncdc commented Jun 29, 2016

Please update the PR title to be more descriptive than just a bug number - thanks!

@ncdc
Copy link
Contributor

ncdc commented Jun 29, 2016

Why do you have a merge commit?

@damemi damemi changed the title Fix for bug #1343358 Bugfix #1343358: Enforce --tty=false flag for "oc debug" Jun 30, 2016
@damemi
Copy link
Contributor Author

damemi commented Jun 30, 2016

Sorry, the merge commit was my mistake. Should be set now

@fabianofranz
Copy link
Member

@damemi Add a link to the bug in the PR description, with the syntax: "Fixes LINK". This will make the robot add a comment to the bug when this gets merged. Also the commit message must be more descriptive about what it's doing, it doesn't have to necessarily mention the bug number. "Enforce --tty=false flag for 'oc debug'" looks like a good commit message, for example.

@fabianofranz
Copy link
Member

@damemi I'd also suggest adding a test in test/cmd/debug.sh.

@damemi damemi force-pushed the tty-bug branch 2 times, most recently from 3a50424 to 723d30d Compare June 30, 2016 20:47
Changes behavior for "oc debug" command, specifically with "--tty" flag. Prior behavior left "--tty" essentially useless, as it was not checked if the user specifically requested to disable TTY by passing "--tty=false" (if the command was run in a terminal, and "--no-tty" was not set to "true", it would always default to a TTY, even with "--tty=false" set.

This change allows both flags to remain for compliance with any other programs that might use them, at least until one is deprecated as it has been suggested that having both is redundant.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1343358
@danmcp
Copy link
Contributor

danmcp commented Jul 11, 2016

@fabianofranz Bump

@fabianofranz
Copy link
Member

[test]

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 313a029

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6906/)

@fabianofranz
Copy link
Member

[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 313a029

@openshift-bot openshift-bot merged commit f7fddd7 into openshift:master Jul 26, 2016
@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6937/)

@fabianofranz
Copy link
Member

@damemi Remember to put the bug ON_QA with a comment and link to this PR. Tks!

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.

5 participants