Skip to content
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

Merged
merged 1 commit into from
Jun 22, 2017

Conversation

zeari
Copy link

@zeari zeari commented Jun 8, 2017

Fix empty settings error in containers topology (#1453 (comment))

The error was:
adc95f80-4480-11e7-86c8-c17a1bb2d003

@himdel Please review
cc @simon3z

@zeari zeari force-pushed the fix_topology_max_items_2 branch from aa29759 to 1468c9d Compare June 8, 2017 09:59
@simon3z
Copy link
Contributor

simon3z commented Jun 8, 2017

@miq-bot assign himdel
cc @himdel

@zeari
Copy link
Author

zeari commented Jun 8, 2017

@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) {
Copy link
Contributor

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..

Copy link
Author

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

Copy link
Contributor

@himdel himdel Jun 13, 2017

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..

Copy link
Author

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.

@miq-bot
Copy link
Member

miq-bot commented Jun 18, 2017

Checked commit zeari@9e3c412 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
0 files checked, 0 offenses detected
Everything looks fine. 🏆

@zeari zeari closed this Jun 22, 2017
@zeari zeari reopened this Jun 22, 2017
'Vm',
'ContainerNode',
'ContainerManager'];
'ContainerGroup',
Copy link
Contributor

@himdel himdel Jun 22, 2017

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).

@himdel himdel added this to the Sprint 64 Ending Jul 3, 2017 milestone Jun 22, 2017
@himdel himdel merged commit d421e54 into ManageIQ:master Jun 22, 2017
simaishi pushed a commit that referenced this pull request Jun 22, 2017
Fix empty settings error in containers topology
(cherry picked from commit d421e54)

https://bugzilla.redhat.com/show_bug.cgi?id=1460382
@simaishi
Copy link
Contributor

Fine backport details:

$ git log -1
commit 14085567d82c02e79458e2d6f673b6ab01199ab3
Author: Martin Hradil <himdel@seznam.cz>
Date:   Thu Jun 22 14:20:18 2017 +0200

    Merge pull request #1507 from zeari/fix_topology_max_items_2
    
    Fix empty settings error in containers topology
    (cherry picked from commit d421e545ea6fb862ad619c581690fdda6262864b)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1460382

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants