Add temporary "silent" flag to `oc login` to allow silent logins to prevent noisy output from clusters with large amounts of prjects #18684

Oats87 · 2018-02-20T21:00:27Z

This simply adds a silent flag to the oc login command to prevent oc from spitting out a ton of extraneous data when logging into a cluster that has a lot of projects accessible to a user.

For BZ #1542326 - https://bugzilla.redhat.com/show_bug.cgi?id=1542326
RFE is within that BZ

pweil- · 2018-02-20T21:03:29Z

/ok-to-test

juanvallejo · 2018-02-20T21:24:45Z

pkg/oc/cli/cmd/login/loginoptions.go

@@ -68,6 +68,9 @@ type LoginOptions struct {

 	CommandName    string
 	RequestTimeout time.Duration
+
+	// Silent login


comment here not needed

juanvallejo · 2018-02-20T21:25:09Z

pkg/oc/cli/cmd/login/loginoptions.go

@@ -405,3 +409,8 @@ func (o *LoginOptions) serverProvided() bool {
 func (o *LoginOptions) tokenProvided() bool {
 	return len(o.Token) > 0
 }
+
+func (o *LoginOptions) silentLogin() bool {


not needed, just use the field value itself

juanvallejo · 2018-02-20T21:25:23Z

pkg/oc/cli/cmd/login/loginoptions.go

-				fmt.Fprintf(o.Out, "  * %s\n", p)
-			} else {
-				fmt.Fprintf(o.Out, "    %s\n", p)
+		if (!o.silentLogin()) {


juanvallejo · 2018-02-20T21:30:41Z

pkg/oc/cli/cmd/login/loginoptions.go

-			} else {
-				fmt.Fprintf(o.Out, "    %s\n", p)
+		if (!o.silentLogin()) {
+			fmt.Fprintf(o.Out, "You have access to the following projects and can switch between them with '%s project <projectname>':\n\n", o.CommandName)


@soltysh @mfojtik while I see the importance behind preventing an output of +1000 projects, perhaps it'd be a better approach to always print this line with the total number of projects You have access to 20 projects .... We could then either print the first 5 or 10 projects, while suggesting the use of oc projects to retrieve the complete list. Thoughts?

While limiting the output for general usage would be great, I don't feel it's a full solution to what my PR is trying to solve.

For those that are service providers and are logging in as cluster-admins or cluster-readers, they'll probably never end up using the first 20 projects so it's useless to print them out to them rather than just allowing them to disable the output of projects on login.

If we added such a flag, I would expect this to print nothing and just have a non-zero exit code on failure.

juanvallejo · 2018-02-20T21:32:21Z

@Oats87 thanks for the PR! You'll need to run ./hack/update-generated-completions.sh and ./hack/update-generated-docs.sh when adding new flags

enj · 2018-02-20T21:58:49Z

This needs to follow existing command convention at the very least:

origin/pkg/oc/admin/policy/cani.go

Line 79 in 73338c4

    
           cmd.Flags().BoolVarP(&o.Quiet, "quiet", "q", o.Quiet, "If true, suppress output and just return the exit code.")

In general I am not a fan of adding this. Just redirect to /dev/null if you do not want to see the output. oc login is not meant to be run by autonomous processes/scripts - it is meant to be run by humans.

/hold

enj · 2018-02-20T22:00:41Z

pkg/oc/cli/cmd/login/login.go

@@ -152,6 +153,8 @@ func (o *LoginOptions) Complete(f *osclientcmd.Factory, cmd *cobra.Command, args
 	o.InsecureTLS = kcmdutil.GetFlagBool(cmd, "insecure-skip-tls-verify")
 	o.Token = kcmdutil.GetFlagString(cmd, "token")

+	o.Silent = kcmdutil.GetFlagBool(cmd, "silent")	


Seems unnecessary, @juanvallejo why is this filling out fields when they should be filled via the pointer stuff?

We still have a number of places that "complete" fields by getting a flag value.
I don't think I'd call it an established convention yet

enj · 2018-02-20T22:04:00Z

pkg/oc/cli/cmd/login/loginoptions.go

-			} else {
-				fmt.Fprintf(o.Out, "    %s\n", p)
+		if (!o.silentLogin()) {
+			fmt.Fprintf(o.Out, "You have access to the following projects and can switch between them with '%s project <projectname>':\n\n", o.CommandName)


If we added such a flag, I would expect this to print nothing and just have a non-zero exit code on failure.

enj · 2018-02-20T22:16:04Z

@Oats87 what exactly do you mean by Add temporary "silent" flag? There is no such thing as a temporary flag because removing or change flags breaks scripts, and thus is considered a breaking API change.

@mfojtik @juanvallejo you guys own the CLI so I will let you decide. This specific command belongs to sig-sec, but I can live with changes if you guys feel strongly enough.

Oats87 · 2018-02-20T22:30:20Z

@enj It is a temporary silent flag because it is not persistent, and thus does not persist if you do "oc login -s" then "oc login"

juanvallejo · 2018-02-20T22:34:45Z

@enj

@mfojtik @juanvallejo you guys own the CLI so I will let you decide. This specific command belongs to sig-sec, but I can live with changes if you guys feel strongly enough.

In general, adding new flags is something I'd be careful about doing.
If the suggested use-case for this can be solved by redirecting a large output to another command,
or by improving the behavior of oc login when a user has access to a large number of projects (see #18684 (comment)) I'd much prefer to do that instead.

enj · 2018-02-20T22:40:06Z

It is a temporary silent flag because it is not persistent, and thus does not persist if you do "oc login -s" then "oc login"

Don't flags, by definition, only last for the invocation of the command?

@juanvallejo maybe it is better to not print the project listing at all when the list has more than 100 items (print "You have access to N projects and the list has been surpressed")? That should basically leave the behavior unchanged for all users except admins (and even then admins with a sufficiently large cluster for this to come into play).

openshift-ci-robot · 2018-02-21T21:52:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Oats87

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

openshift-ci-robot · 2018-02-21T22:37:48Z

@Oats87: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/extended_conformance_install	`fa82543`	link	`/test extended_conformance_install`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Automatic merge from submit-queue. Surpress project list on login if you have access to greater than 50 projects As a compromise to #18684 we are going to surpress the project list functionality if the number of projects available to a user is greater than 50 Original RFE was at BZ #1542326 - https://bugzilla.redhat.com/show_bug.cgi?id=1542326 RFE is within that BZ, but there was a compromise.

openshift-ci-robot requested review from enj and soltysh February 20, 2018 21:00

openshift-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 20, 2018

pweil- assigned juanvallejo Feb 20, 2018

openshift-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 20, 2018

juanvallejo reviewed Feb 20, 2018

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 20, 2018

enj suggested changes Feb 20, 2018

View reviewed changes

Oats87 closed this Feb 21, 2018

Oats87 force-pushed the master branch from 1ae2adf to fa82543 Compare February 21, 2018 21:52

openshift-ci-robot added the retest-not-required-docs-only label Feb 21, 2018

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 21, 2018

Oats87 mentioned this pull request Feb 21, 2018

Surpress project list on login if you have access to greater than 50 projects #18706

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add temporary "silent" flag to `oc login` to allow silent logins to prevent noisy output from clusters with large amounts of prjects #18684

Add temporary "silent" flag to `oc login` to allow silent logins to prevent noisy output from clusters with large amounts of prjects #18684

Oats87 commented Feb 20, 2018

pweil- commented Feb 20, 2018

juanvallejo Feb 20, 2018

juanvallejo Feb 20, 2018

juanvallejo Feb 20, 2018

juanvallejo Feb 20, 2018

Oats87 Feb 20, 2018

enj Feb 20, 2018

juanvallejo commented Feb 20, 2018

enj commented Feb 20, 2018

enj Feb 20, 2018

juanvallejo Feb 20, 2018

enj Feb 20, 2018

enj commented Feb 20, 2018

Oats87 commented Feb 20, 2018

juanvallejo commented Feb 20, 2018

enj commented Feb 20, 2018

openshift-ci-robot commented Feb 21, 2018

openshift-ci-robot commented Feb 21, 2018 •

edited

Loading

Add temporary "silent" flag to oc login to allow silent logins to prevent noisy output from clusters with large amounts of prjects #18684

Add temporary "silent" flag to oc login to allow silent logins to prevent noisy output from clusters with large amounts of prjects #18684

Conversation

Oats87 commented Feb 20, 2018

pweil- commented Feb 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanvallejo commented Feb 20, 2018

enj commented Feb 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enj commented Feb 20, 2018

Oats87 commented Feb 20, 2018

juanvallejo commented Feb 20, 2018

enj commented Feb 20, 2018

openshift-ci-robot commented Feb 21, 2018

openshift-ci-robot commented Feb 21, 2018 • edited Loading

Add temporary "silent" flag to `oc login` to allow silent logins to prevent noisy output from clusters with large amounts of prjects #18684

Add temporary "silent" flag to `oc login` to allow silent logins to prevent noisy output from clusters with large amounts of prjects #18684

openshift-ci-robot commented Feb 21, 2018 •

edited

Loading