-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Limit the number of events and deployments displayed in dc describer #9633
Conversation
@Kargakis PTAL |
counter++ | ||
} | ||
// Only show last three deployments here | ||
if counter == 2 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment that this number needs to be on par with the respective number in oc status
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Kargakis I can make it a const
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I didn't realize we are on the same package. Yes, please, make it a constant.
const ( | ||
// MaxDisplayDeployments is the number of deployments to show when describing | ||
// deployment configuration. | ||
// TODO: Use this constant when limiting the number of deployments displayed in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plug this in oc status
while here. Also we don't need to export any of these constants.
@Kargakis fixed the count and plumbed |
const ( | ||
// maxDisplayDeployments is the number of deployments to show when describing | ||
// deployment configuration. | ||
// TODO: Use this constant when limiting the number of deployments displayed in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the TODO
One last comment, LGTM otherwise |
@Kargakis done. will merge on green tests |
[merge] |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/5670/) (Image: devenv-rhel7_4502) |
Evaluated for origin merge up to 18929ed |
[Test]ing while waiting on the merge queue |
Evaluated for origin test up to 18929ed |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/5670/) |
Fixes: #9604
This PR will limit the events displayed to last 4 and also the deployments displayed in describe to last 3.