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

Add top row information into the storage manager quadicons #4987

Merged
merged 1 commit into from
Dec 4, 2018

Conversation

skateman
Copy link
Member

@skateman skateman commented Nov 26, 2018

There's no separate STI class for block & object storage managers, so I had to do this ugly hack while adding the missing information to the quadrants.

Before:
screenshot from 2018-11-26 16-45-23
screenshot from 2018-11-26 16-45-46

After:
screenshot from 2018-11-26 16-40-27
screenshot from 2018-11-29 16-23-03

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1650086

@miq-bot add_label GTLs, hammer/yes, bug
@miq-bot add_reviewer @epwinchell

@JPrause
Copy link
Member

JPrause commented Nov 27, 2018

@miq-bot add_label blocker

Copy link
Contributor

@epwinchell epwinchell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested. Looks good.

@skateman
Copy link
Member Author

I'm still missing the object storage quadrants from @Loicavenel.

@Loicavenel
Copy link

@skateman there is only one option available: Cloud object store containers .. Put that on the top left

@skateman skateman changed the title [WIP] Add top row information into the storage manager quadicons Add top row information into the storage manager quadicons Nov 29, 2018
@skateman
Copy link
Member Author

Thanks @Loicavenel, updated

@martinpovolny
Copy link
Member

@skateman : a small spec? for the new method?

@martinpovolny martinpovolny self-assigned this Nov 30, 2018
@skateman
Copy link
Member Author

skateman commented Nov 30, 2018

@martinpovolny there's no new method and I'm against writing specs for declarative stuff like decorators. I can surely test if 1 = 1, but I think just turning off the coverage for the declarations makes more sense. Also the asset pipeline is turned off in testing, so some of the decorator specs would be really just 1 = 1...

This special case of storage with the decision logic is wrong and I would like to get rid of it, just haven't figured out how to do so.

@martinpovolny
Copy link
Member

@skateman : I'm sorry, but it's not that simple. You are adding code to a method that has no specs. You are adding conditions, you are calling other methods. This method is no longer a trivial declaration.

I suggest that you add a very simple spec. I do not actually care what the spec tests, I don't want you to test this condition or that. I want at least this piece of code to be called from the test suite. I want a test that failed if I remove the code or if I call a non-existent method there.

For example if you create a situation where supports_block_storage? would be true you and call the method just once, you will get > 50% of the lines executed. The check if a hash with 2 expected keys is returned and we have at least something.

Or make sure that the method is called as part of a more complex scheme, where more of decorator would be used.

Pls, do not make PRs that decrease test coverage. Not even a little ;-)

@skateman
Copy link
Member Author

skateman commented Dec 4, 2018

@martinpovolny done

@miq-bot
Copy link
Member

miq-bot commented Dec 4, 2018

Checked commit skateman@e16922e with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. ⭐

@martinpovolny martinpovolny merged commit c6662af into ManageIQ:master Dec 4, 2018
@martinpovolny martinpovolny added this to the Sprint 101 Ending Dec 17, 2018 milestone Dec 4, 2018
simaishi pushed a commit that referenced this pull request Dec 4, 2018
Add top row information into the storage manager quadicons

(cherry picked from commit c6662af)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1650086
@skateman skateman deleted the storage-manager-quad branch December 4, 2018 14:48
@simaishi
Copy link
Contributor

simaishi commented Dec 4, 2018

Hammer backport details:

$ git log -1
commit 59ca8e385312fef66beb20d36381204b53702f16
Author: Martin Povolny <mpovolny@redhat.com>
Date:   Tue Dec 4 15:43:50 2018 +0100

    Merge pull request #4987 from skateman/storage-manager-quad
    
    Add top row information into the storage manager quadicons
    
    (cherry picked from commit c6662afdd9e0f71f11aff58e8486c4f956dfae06)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1650086

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.

7 participants