Surpress project list on login if you have access to greater than 50 projects by Oats87 · Pull Request #18706 · openshift/origin

Oats87 · 2018-02-21T22:14:27Z

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.

juanvallejo · 2018-02-21T23:00:18Z

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

-				fmt.Fprintf(o.Out, "  * %s\n", p)
-			} else {
-				fmt.Fprintf(o.Out, "    %s\n", p)
+		if len(projectsItems) > 50 {


Please make the 50 a constant maxProjectItemLimit (or a similar name) at the top of this file.

juanvallejo · 2018-02-21T23:01:08Z

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

-			} else {
-				fmt.Fprintf(o.Out, "    %s\n", p)
+		if len(projectsItems) > 50 {
+			fmt.Fprintf(o.Out, "You have access to %d projects, the list has been surpressed. You can list all projects with '%s projects'\n", len(projectsItems), o.CommandName)


I think it'd be worth adding a comment before this line, explaining why we are doing this. That way we can prevent someone in the future from accidentally reverting this

juanvallejo · 2018-02-21T23:01:57Z

/lgtm
Minor comments, otherwise lgtm.

@enj for approval tag

enj · 2018-02-21T23:36:26Z

@Oats87 please squash into one commit and include the text Bug 1542326 in the commit message.

enj · 2018-02-21T23:45:49Z

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

+
+		// Surpress project listing if the number of projects available to the user is greater than the threshold. Prevents unnecessarily noisy logins on clusters with large numbers of projects
+		if len(projectsItems) > projectsItemsSurpressThreshold {
+			fmt.Fprintf(o.Out, "You have access to %d projects, the list has been surpressed. You can list all projects with '%s projects'\n", len(projectsItems), o.CommandName)


nit: have this end with two newlines to match:

normal user:

Login successful. You have access to the following projects and can switch between them with 'oc project <projectname>': abb abbb * abbbb Using project "abbbb".

admin:

Logged into "https://10.13.129.85:8443" as "system:admin" using existing credentials. You have access to 123 projects, the list has been surpressed. You can list all projects with 'oc projects' Using project "abbbb".

I removed that second newline while validating the Fprintf line with @juanvallejo ; should I add it back in?

@enj I initially saw that extra newline as part of the "padding" for a list of projects. Since we are not listing projects in that second example, I thought to omit it instead. I do not feel strongly about this one way or another

enj · 2018-02-22T00:03:32Z

/lgtm

Oats87 · 2018-02-22T01:14:37Z

Sorry @enj @juanvallejo I had to push one last time because suppress was misspelled (once it was misspelled once, I took off running with it without batting an eye)

enj · 2018-02-22T02:12:21Z

@Oats87 I don't we want that information in the commit message though 😄

enj · 2018-02-22T02:13:04Z

@Oats87 also, you need to run hack/verify-gofmt.sh | xargs -n 1 gofmt -s -w

Added conditional to project list that will suppress project list output on login if the list is greater than 50. Bug 1542326

Oats87 · 2018-02-22T02:49:16Z

@enj Haha, understood. I went ahead and pulled that line out.

Did you see a format error on the file? Thanks for the command, I didn't know about that.

When I ran it, it didn't say any files were not formatted correctly, and I diff'ed the loginoptions.go file between the output of gofmt -s and the existing file and there were no differences

enj · 2018-02-22T04:21:46Z

/ok-to-test
/lgtm
/refresh

openshift-ci-robot · 2018-02-22T04:21:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enj, juanvallejo, Oats87

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

Details

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

~~pkg/oc/OWNERS~~ [enj]

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

openshift-bot · 2018-02-22T06:29:41Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2018-02-22T10:31:36Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-merge-robot · 2018-02-22T12:07:31Z

/test all [submit-queue is verifying that this PR is safe to merge]

openshift-merge-robot · 2018-02-22T13:34:25Z

Automatic merge from submit-queue.

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 21, 2018

openshift-ci-robot requested review from enj and juanvallejo February 21, 2018 22:14

juanvallejo reviewed Feb 21, 2018

View reviewed changes

openshift-ci-robot assigned juanvallejo Feb 21, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 21, 2018

openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 21, 2018

enj reviewed Feb 21, 2018

View reviewed changes

Oats87 force-pushed the surpress-over-50 branch 3 times, most recently from 769bdfd to 605d7bb Compare February 21, 2018 23:54

openshift-ci-robot assigned enj Feb 22, 2018

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 22, 2018

Oats87 force-pushed the surpress-over-50 branch from 605d7bb to 75edf7c Compare February 22, 2018 01:10

openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 22, 2018

Oats87 force-pushed the surpress-over-50 branch from 75edf7c to 99ac2be Compare February 22, 2018 01:12

Oats87 force-pushed the surpress-over-50 branch from 99ac2be to 8125376 Compare February 22, 2018 02:47

Prevent login spam on large clusters

8125376

Added conditional to project list that will suppress project list output on login if the list is greater than 50. Bug 1542326

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 22, 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 22, 2018

openshift-merge-robot merged commit da86116 into openshift:master Feb 22, 2018

Conversation

Oats87 commented Feb 21, 2018

Uh oh!

juanvallejo Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

juanvallejo Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

juanvallejo commented Feb 21, 2018

Uh oh!

enj commented Feb 21, 2018

Uh oh!

enj Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

Oats87 Feb 21, 2018

Choose a reason for hiding this comment

Uh oh!

juanvallejo Feb 22, 2018

Choose a reason for hiding this comment

Uh oh!

enj commented Feb 22, 2018

Uh oh!

Oats87 commented Feb 22, 2018

Uh oh!

enj commented Feb 22, 2018

Uh oh!

enj commented Feb 22, 2018

Uh oh!

Oats87 commented Feb 22, 2018

Uh oh!

enj commented Feb 22, 2018

Uh oh!

openshift-ci-robot commented Feb 22, 2018

Uh oh!

openshift-bot commented Feb 22, 2018

Uh oh!

openshift-bot commented Feb 22, 2018

Uh oh!

openshift-merge-robot commented Feb 22, 2018

Uh oh!

openshift-merge-robot commented Feb 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants