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

support -f flag for --follow and -w for --wait in start-build #9963

Merged
merged 1 commit into from
Jul 27, 2016

Conversation

bparees
Copy link
Contributor

@bparees bparees commented Jul 20, 2016

@smarterclayton ptal, not sure if there was some good reason we never allowed this before?

@@ -91,7 +91,7 @@ func NewCmdStartBuild(fullName string, f *clientcmd.Factory, in io.Reader, out i
cmd.Flags().StringSliceVarP(&o.Env, "env", "e", o.Env, "Specify key value pairs of environment variables to set for the build container.")
cmd.Flags().StringVar(&o.FromBuild, "from-build", o.FromBuild, "Specify the name of a build which should be re-run")

cmd.Flags().BoolVar(&o.Follow, "follow", o.Follow, "Start a build and watch its logs until it completes or fails")
cmd.Flags().BoolVarP(&o.Follow, "follow", "f", o.Follow, "Start a build and watch its logs until it completes or fails")
Copy link
Contributor

Choose a reason for hiding this comment

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

-f is generally for file... I'd expect that to map to --from-file, if anything

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah but -f is follow for oc logs.

and --follow seems more commonly used than --from-file, so i felt it deserved precedence.

@bparees bparees changed the title support -f flag for --follow in start-build support -f flag for --follow amd -w for --wait in start-build Jul 20, 2016
@bparees bparees changed the title support -f flag for --follow amd -w for --wait in start-build support -f flag for --follow and -w for --wait in start-build Jul 20, 2016
@liggitt
Copy link
Contributor

liggitt commented Jul 20, 2016

https://github.com/kubernetes/kubernetes/blob/master/docs/devel/kubectl-conventions.md#flag-conventions

-f: Resource file
also used for --follow in logs, but should be deprecated in favor of -F

@bparees
Copy link
Contributor Author

bparees commented Jul 20, 2016

ok, so would you support this if i used -F instead of -f?

@liggitt
Copy link
Contributor

liggitt commented Jul 20, 2016

yup. single letters are scarce and we want to make them as consistent as possible across as many commands as possible. ideally, we could support -F and -f concurrently for the log commands as well

@bparees
Copy link
Contributor Author

bparees commented Jul 20, 2016

i've updated it to -F here.

@bparees
Copy link
Contributor Author

bparees commented Jul 20, 2016

[test]

@bparees
Copy link
Contributor Author

bparees commented Jul 21, 2016

flake #8427
flake #9490

[test]

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 555ce55

@openshift-bot
Copy link
Contributor

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

@bparees
Copy link
Contributor Author

bparees commented Jul 22, 2016

@openshift/api-review bump

@deads2k
Copy link
Contributor

deads2k commented Jul 25, 2016

Hold on using -F. That could end up colliding with --files-from. kubernetes/kubernetes#29466

@bparees
Copy link
Contributor Author

bparees commented Jul 25, 2016

@deads2k so -F came about because of @liggitt's comment here:
#9963 (comment)

though on re-reading that link it's not clear to me if k8s is saying "-f for --follow should be deprecated in favor of -F", or "-f for files should be deprecated in favor of -F"... I assume the former, but it's not clearly stated)

@bparees
Copy link
Contributor Author

bparees commented Jul 26, 2016

@openshift/api-review can we get a consensus here on how to move forward?

@deads2k
Copy link
Contributor

deads2k commented Jul 26, 2016

@openshift/api-review can we get a consensus here on how to move forward?

He removed the -F as the shortname for the flag upstream. We need to make sure that common upstream CLI reviewers don't let it slip back in.

I don't have any objection.

@smarterclayton
Copy link
Contributor

I'm tempted to remove -f for --files here, since 99% of the time users want tail behavior.

How many times do you think our users will want to follow builds, vs pass files vs. -f.

@smarterclayton
Copy link
Contributor

Let's just use -F. Users will hate us, but they hate us anyway.

@bparees
Copy link
Contributor Author

bparees commented Jul 27, 2016

@smarterclayton does that constitute api approval? (begrudging approval?)

@smarterclayton
Copy link
Contributor

Yes.

@bparees
Copy link
Contributor Author

bparees commented Jul 27, 2016

begrudging [merge]

@openshift-bot
Copy link
Contributor

openshift-bot commented Jul 27, 2016

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6960/) (Image: devenv-rhel7_4677)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 555ce55

@openshift-bot openshift-bot merged commit bb2f9fc into openshift:master Jul 27, 2016
@bparees bparees deleted the build_follow branch July 28, 2016 21:03
@0xmichalis
Copy link
Contributor

Don't we use -w for --watch elsewhere? I guess we also do for --wait?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants