Surpress project list on login if you have access to greater than 50 projects #18706
Conversation
openshift-ci-robot
added
needs-ok-to-test
pkg/oc/cli/cmd/login/loginoptions.go
Outdated
| fmt.Fprintf(o.Out, " * %s\n", p) | ||
| } else { | ||
| fmt.Fprintf(o.Out, " %s\n", p) | ||
| if len(projectsItems) > 50 { |
There was a problem hiding this comment.
Please make the 50 a constant maxProjectItemLimit (or a similar name) at the top of this file.
pkg/oc/cli/cmd/login/loginoptions.go
Outdated
| } 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) |
There was a problem hiding this comment.
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
|
/lgtm @enj for approval tag |
openshift-merge-robot
removed
the
lgtm
|
@Oats87 please squash into one commit and include the text |
pkg/oc/cli/cmd/login/loginoptions.go
Outdated
|
|
||
| // 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) |
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
I removed that second newline while validating the Fprintf line with @juanvallejo ; should I add it back in?
There was a problem hiding this comment.
@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
Oats87
force-pushed
the
surpress-over-50
branch
3 times, most recently
from
769bdfd
to
605d7bb
Compare
|
/lgtm |
openshift-ci-robot
added
lgtm
Oats87
force-pushed
the
surpress-over-50
branch
from
605d7bb
to
75edf7c
Compare
openshift-merge-robot
removed
the
lgtm
Oats87
force-pushed
the
surpress-over-50
branch
from
75edf7c
to
99ac2be
Compare
|
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) |
|
@Oats87 I don't we want that information in the commit message though 😄 |
|
@Oats87 also, you need to run |
Oats87
force-pushed
the
surpress-over-50
branch
from
99ac2be
to
8125376
Compare
Added conditional to project list that will suppress project list output on login if the list is greater than 50. Bug 1542326
|
@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 |
|
/ok-to-test |
openshift-ci-robot
removed
the
needs-ok-to-test
|
[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.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue. |
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.