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 topology max items feature #1453

Merged
merged 1 commit into from
May 29, 2017

Conversation

zeari
Copy link

@zeari zeari commented May 29, 2017

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1455892
Fixes the feature introduced in #95. Setting an item limit for topology didnt work since theres inconsistency with the naming of the limit.
items is the correct term so what this PR does is s/objects/items/g to fix the bug.

@zeari
Copy link
Author

zeari commented May 29, 2017

@himdel @martinpovolny Please review.

@miq-bot
Copy link
Member

miq-bot commented May 29, 2017

Checked commit zeari@5add6f4 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 🍪

@martinpovolny martinpovolny merged commit a26ddfc into ManageIQ:master May 29, 2017
@martinpovolny martinpovolny added this to the Sprint 62 Ending Jun 5, 2017 milestone May 29, 2017
@zeari
Copy link
Author

zeari commented May 29, 2017

Thanks @martinpovolny !

@martinpovolny martinpovolny self-assigned this May 29, 2017
@simon3z
Copy link
Contributor

simon3z commented May 29, 2017

@zeari @martinpovolny should we handle the error notification (in the Toplogy page) about containers_max_items?

screenshot from 2017-05-29 15-07-39

@himdel
Copy link
Contributor

himdel commented Jun 5, 2017

@simon3z Definitely :). When is that happening? But looks like the code just dies if data.data.settings is null, so if that can happen, the code should be handling it, yes.

@simon3z
Copy link
Contributor

simon3z commented Jun 5, 2017

@simon3z Definitely :). When is that happening? But looks like the code just dies if data.data.settings is null, so if that can happen, the code should be handling it, yes.

@himdel I just navigate to the Toplogy page (fresh install, never configured a max item setting).

@himdel
Copy link
Contributor

himdel commented Jun 5, 2017

Aah, makes sense, thanks .. looks like a simple (data.data.settings || {}).containers_max_items would do the trick.

Or maybe move it to that if and do } else if (data.data.settings && data.data.settings.containers_max_items) {.

@simon3z
Copy link
Contributor

simon3z commented Jun 5, 2017

Aah, makes sense, thanks .. looks like a simple (data.data.settings || {}).containers_max_items would do the trick.

Or maybe move it to that if and do } else if (data.data.settings && data.data.settings.containers_max_items) {.

@zeari @himdel can anyone handle this?

@zeari
Copy link
Author

zeari commented Jun 8, 2017

@zeari @himdel can anyone handle this?

looking into it.

simaishi pushed a commit that referenced this pull request Jun 9, 2017
@simaishi
Copy link
Contributor

simaishi commented Jun 9, 2017

Fine backport details:

$ git log -1
commit a3ea4bbb71ca93c7f5866ddc1b914e5c4054dc96
Author: Martin Povolny <[email protected]>
Date:   Mon May 29 13:44:47 2017 +0200

    Merge pull request #1453 from zeari/topology_fix_max_items
    
    fix topology max items feature
    (cherry picked from commit a26ddfcf769c9fcde2978bc2b39841ecbf8c7790)
    
    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.

6 participants