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 support for displaying storage device details #3830

Merged
merged 2 commits into from May 3, 2018

Conversation

skovic
Copy link

@skovic skovic commented Apr 23, 2018

This PR adds support for displaying details about storage devices associated with physical servers. A link displaying the number of storage devices associated with a server will be displayed on the Physical Server Summary page. Clicking on this link will take the user to a page that displays a list of a server's storage devices. Clicking on one of these devices takes the user to the Storage Device Summary page for that device.

image

image

image

This PR depends on ManageIQ/manageiq#17332

@skovic
Copy link
Author

skovic commented Apr 23, 2018

@miq-bot assign_label wip

@miq-bot
Copy link
Member

miq-bot commented Apr 23, 2018

@skovic unrecognized command 'assign_label', ignoring...

Accepted commands are: add_label, add_reviewer, assign, close_issue, move_issue, remove_label, rm_label, set_milestone

@skovic
Copy link
Author

skovic commented Apr 23, 2018

@miq-bot add_label wip

@miq-bot miq-bot changed the title Add support for displaying storage device details [WIP] Add support for displaying storage device details Apr 23, 2018
@miq-bot miq-bot added the wip label Apr 23, 2018
@miq-bot
Copy link
Member

miq-bot commented Apr 23, 2018

Checked commits skovic/manageiq-ui-classic@c6b4f29~...b0ead7a with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🍪

@skovic
Copy link
Author

skovic commented Apr 24, 2018

@miq-bot remove_label wip

@miq-bot miq-bot changed the title [WIP] Add support for displaying storage device details Add support for displaying storage device details Apr 24, 2018
@miq-bot miq-bot removed the wip label Apr 24, 2018
@skovic
Copy link
Author

skovic commented Apr 24, 2018

@miq-bot add_label compute/physical infrastructure

@skovic
Copy link
Author

skovic commented Apr 24, 2018

@agrare This PR is also ready for review. Thanks

@agrare
Copy link
Member

agrare commented Apr 24, 2018

@miq-bot assign @himdel

@himdel
Copy link
Contributor

himdel commented Apr 25, 2018

LGTM :)

(but not sure this should even be the same class - ManageIQ/manageiq#17332 (comment) )

@skovic
Copy link
Author

skovic commented Apr 26, 2018

@himdel Looks like the PR this one depends on got merged. Any other changes needed in this PR? Thanks

@himdel
Copy link
Contributor

himdel commented May 2, 2018

@skovic The code looks good now, I think there should be nothing blocking this :).

Unfortunately that's not quite true: I have to be able to test this and I don't have the data.

Do you have any publicly-facing provider that can be used for testing please?
Or a script that adds a sample physical provider + server with all the relevant data to the db?

(Usually we either have a testing provider, or a process of setting one up is documented in https://github.com/ManageIQ/guides/tree/master/providers , or there's a script like openshift set-miq-providers that just adds the right DB entries. But no such luck for Lenovo.)

Thanks

@skovic
Copy link
Author

skovic commented May 2, 2018

@himdel Would it suffice for me to walk you through the UI in a Blue Jeans or Skype session?

@himdel
Copy link
Contributor

himdel commented May 2, 2018

@skovic Not really, sorry.

But if you can convince another member of your team to test this in the UI (and write it here), I'm OK with merging :)

(The point is: every PR should be tested in the UI by somebody other than the author. We don't do this as a strict requirement, as most of the time, it's easier to just test & merge than to ask & wait, but in cases like this... ((It's much easier to prevent those "but it works on my machine" kind of bugs if there's at least two people with non-identical databases who managed to get it working.:)))

@skovic
Copy link
Author

skovic commented May 2, 2018

@rodneyhbrown7 Can you test these changes out when you get a chance? Thanks

@rodneyhbrown7
Copy link

LGTM - I was able to verify the function in this PR on my development appliance.

@himdel himdel merged commit 3de8562 into ManageIQ:master May 3, 2018
@himdel himdel added this to the Sprint 85 Ending May 7, 2018 milestone May 3, 2018
@himdel
Copy link
Contributor

himdel commented May 3, 2018

Thanks both of you! :) Merged 👍

(Just note that something like this would be needed for every PR that's specific to physical infra, so it might be a good idea to get some testing data either way :).)

@himdel himdel removed this from the Sprint 85 Ending May 7, 2018 milestone May 3, 2018
@himdel himdel added this to the Sprint 85 Ending May 7, 2018 milestone May 3, 2018
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.

None yet

5 participants