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 Redfish provider creation dialog #3912

Merged
merged 1 commit into from
Jun 6, 2018
Merged

Conversation

tadeboro
Copy link
Contributor

@tadeboro tadeboro commented May 8, 2018

This pull requests contains UI modifications that are needed in order to be able to create new Redfish provider from the MIQ web UI.

/cc @gberginc @gtanzillo @Fryguy

@gberginc
Copy link
Contributor

gberginc commented May 9, 2018

Marking WIP as it depends on the provider actually being migrated to ManageIQ.

@miq-bot add_label wip

@miq-bot miq-bot changed the title Add Redfish provider creation dialog [WIP] Add Redfish provider creation dialog May 9, 2018
@miq-bot miq-bot added the wip label May 9, 2018
@tadeboro
Copy link
Contributor Author

@miq-bot remove_label wip

@tadeboro
Copy link
Contributor Author

Related bugzilla entry: https://bugzilla.redhat.com/show_bug.cgi?id=1574808

@miq-bot miq-bot changed the title [WIP] Add Redfish provider creation dialog Add Redfish provider creation dialog May 15, 2018
@miq-bot miq-bot removed the wip label May 15, 2018
@mzazrivec
Copy link
Contributor

mzazrivec commented May 16, 2018

Is there any backend / provider PR that this PR depends on?

@tadeboro
Copy link
Contributor Author

@mzazrivec Sorry, forgot to add this info. Relevant core pull request is ManageIQ/manageiq#17392, so this will need to wait for that pull request to get merged.

@miq-bot add_label wip

@miq-bot miq-bot changed the title Add Redfish provider creation dialog [WIP] Add Redfish provider creation dialog May 16, 2018
@miq-bot miq-bot added the wip label May 16, 2018
Copy link
Contributor

@gberginc gberginc left a comment

Choose a reason for hiding this comment

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

Please check my comment regarding the security protocols.

@@ -33,7 +34,7 @@
"selectpicker-for-select-tag" => "",
"prefix" => prefix.to_s,
"reset-validation-status" => "#{prefix}_auth_status")
.col-md-8{"ng-if" => "emsCommonModel.emstype == 'nuage_network'"}
.col-md-8{"ng-if" => "emsCommonModel.emstype == 'nuage_network' || emsCommonModel.emstype == 'redfish_ph_infra'"}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would separate this now. Even if the set of security options is compatible for Nuage and Redfish, the latter does not support AMQP for the eventing (the next row checks whether the security protocols are displayed for AMQP tab or for the main tab; or at least, this is my understanding).

Thus, I suggest we create a new block specifically for the Redfish provider. Once events are going to be supported, we will add the security options for that as well (if necessary).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a remnant from "I just want something to show on UI" times that I forgot to clean up properly. I will fix this by creating new Redfish-only block that will use dedicated security protocol list.

@@ -491,6 +491,7 @@
%div{"ng-if" => "#{ng_model}.emstype == 'gce' || " + |
"#{ng_model}.emstype == 'scvmm' || " + |
"#{ng_model}.emstype == 'lenovo_ph_infra' || " + |
"#{ng_model}.emstype == 'redfish_ph_infra' || " + |
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose this is correct place to put it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is correct in the sense that the following code hides all the tabs that we do not need. I considered adding a separate block for Redfish provider, but I decided against it when I saw that other providers are already reusing the configuration block. Besides, we can always create a new block if/when we actually need to made some customizations.

Copy link
Contributor

@gberginc gberginc left a comment

Choose a reason for hiding this comment

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

Thanks @tadeboro.

Please remove wip and assign this PR to @mzazrivec if dependencies are now satisfied.

@tadeboro
Copy link
Contributor Author

@miq-bot remove_label wip
@miq-bot assign @mzazrivec

@miq-bot miq-bot changed the title [WIP] Add Redfish provider creation dialog Add Redfish provider creation dialog May 18, 2018
@miq-bot miq-bot removed the wip label May 18, 2018
@mzazrivec
Copy link
Contributor

@epwinchell Could you please look at the graphics part of this PR? Thanks.

@tadeboro
Copy link
Contributor Author

Some screenshots of UI changes in action:

select-provider-type

new-redfish-provider-form

security-protocol-selection

validation

provider-created

@martinpovolny
Copy link
Member

ping @terezanovotna

@terezanovotna
Copy link

waiting to get an MIQ environment where I can see these edits. Currently getting server error.

@epwinchell
Copy link
Contributor

@tadeboro Please use a symlink for "vendor-redfish_ph_infra.svg" that points to "vendor-redfish." Thanks.

cc \ @skateman

@tadeboro
Copy link
Contributor Author

@epwinchell I never noticed all those symlinks. I replaced the app/assets/images/svg/vendor-redfish_ph_infra.svg with symlink to app/assets/images/svg/vendor-redfish.svg as instructed.

@epwinchell
Copy link
Contributor

@tadeboro Thank you

@epwinchell
Copy link
Contributor

@miq-bot add_reviewer @epwinchell

@miq-bot miq-bot requested a review from epwinchell May 25, 2018 00:20
@terezanovotna
Copy link

The dialog is being consistent with the current provider form, so @miq-bot remove_labels ux/review
@miq-bot add_labels ux/approved

(as a side note, the provider form itself has some issues, but that's a different topic)

@terezanovotna
Copy link

@miq-bot remove_label ux/review

@himdel
Copy link
Contributor

himdel commented May 28, 2018

LGTM, do we have any servers to test this with?

@tadeboro
Copy link
Contributor Author

@himdel Any server that supports Redfish will do. But since those are not the easiest things to come by, I will prepare a sanitized recording of a live server that can be replayed by the mock server. I will post the relevant links here as soon as I have something ready.

@himdel
Copy link
Contributor

himdel commented May 28, 2018

Perfect, thanks @tadeboro :)

@tadeboro
Copy link
Contributor Author

As promised, I prepared a static recording of the Redfsh-capable system and the mock server that is capable of replaying those responses. All of this is available in redfish-recordings repo. Repository also contains short instructions on how to get the server running.

This server has enough functionality implemented to be able to serve as a substitute for the real thing when developing Redfish provider for MIQ. At the moment, authorization support is only partialy implemented in mock server (any username/password is accepted), so credentials entered in new provider form can be arbitrary.

@miq-bot
Copy link
Member

miq-bot commented Jun 4, 2018

This pull request is not mergeable. Please rebase and repush.

This commit introduces minimal changes that are needed in order to be
able to add Redfish provider to ManageIQ.
@miq-bot
Copy link
Member

miq-bot commented Jun 5, 2018

Checked commit xlab-si@b3cbf19 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 1 offense detected

**

  • 💣 💥 🔥 🚒 - Linter/Haml - Linter::Haml STDERR:
warning: parser/current is loading parser/ruby23, which recognizes
warning: 2.3.6-compliant syntax, but you are running 2.3.3.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.

@mzazrivec
Copy link
Contributor

The provider creation dialog seems to be working all right.

I noticed few other things though:

  1. Missing display name for the new physical server. I created Add display name for Physical Server (Redfish) manageiq#17532 for this.

  2. With the mock server & data provided, I'm getting the following error when navigating to physical server summary screen:

 Started GET "/physical_server/show/136"
...
FATAL -- : Error caught: [ActionView::Template::Error] undefined method `disk_capacity' for nil:NilClass
manageiq-ui-classic/app/helpers/physical_server_helper/textual_summary.rb:96:in `textual_capacity'
  1. Provider topology isn't working as expected: the icons seem to be missing, the graph wouldn't show object names.

Given the title of this PR says "provider creation dialog" I can merge this PR as long as the other issues will be fixed in later PRs. Is that OK with you @tadeboro ?

@tadeboro
Copy link
Contributor Author

tadeboro commented Jun 6, 2018

Thanks @mzazrivec for your comments and help with display name.

The error that you are seeing when navigating to server summary page: this is a know "bug" and is caused by the missing inventory. This will correct itself once we add hardware collection to the inventory refresh to the Redfish provider.

As for the topology, I notice that icons do not show up at first, but if I interact a bit with the UI, they do eventually show.

That being said, I would be overwhelmed it this PR could be merged.

@mzazrivec mzazrivec added this to the Sprint 88 Ending Jun 18, 2018 milestone Jun 6, 2018
@mzazrivec mzazrivec merged commit 112cdd5 into ManageIQ:master Jun 6, 2018
@tadeboro tadeboro deleted the redfish branch June 6, 2018 10:54
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.

9 participants