-
Notifications
You must be signed in to change notification settings - Fork 356
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
Fix empty settings error in containers topology #1507
Conversation
aa29759
to
1468c9d
Compare
@miq-bot add_label topology, bug, compute/containers, fine/yes |
$scope.kinds = topologyService.reduce_kinds($scope.items, $scope.kinds, size_limit, remove_hierarchy); | ||
} else if (data.data.settings && data.data.settings.containers_max_items) { | ||
var size_limit = data.data.settings.containers_max_items; | ||
if (size_limit > 0) { |
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.
Are you expecting containers_max_items
to be negative or non-numeric? If not, this condition is superfluous..
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.
containers_max_items
could be zero which means unlimited
. in that case we dont need to do anything. i can consider that in reduce_kinds
in topology_service.js instead
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.
All I meant is..
else if (data.data.settings && data.data.settings.containers_max_items) {
will let through any non-zero number (but won't let 0 in), so
if (size_limit > 0) {
is useful only for stuff like -1 > 0
or "foo" > 0
but won't change anything when size_limit
is a non-negative number..
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.
ah, ok. so im not expecting anything negative/non-number so im removing that.
1468c9d
to
9e3c412
Compare
Checked commit zeari@9e3c412 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
'Vm', | ||
'ContainerNode', | ||
'ContainerManager']; | ||
'ContainerGroup', |
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.
Note this is the wrong align for JS arrays. It was right before (except for the ending ]
which should be on its own line).
Fix empty settings error in containers topology (cherry picked from commit d421e54) https://bugzilla.redhat.com/show_bug.cgi?id=1460382
Fine backport details:
|
Fix empty settings error in containers topology (#1453 (comment))
The error was:
@himdel Please review
cc @simon3z