React Component that triggers Provisioning Request

[miha-plesko]

With this commit we implement the UI for Redfish physical server provisioning. User is offered to pick PXE Image and Customization template. When she clicks "Provision" a Request is created for her where she can then track provisioning status.

Depends on:

• Support physical server provisioning manageiq-api#578

[miha-plesko]

@Hyperkid123 @martinpovolny kindly ask for your opinion. Is similar to what we did for Nuage some time ago ManageIQ/manageiq-providers-nuage#118. I'll provide unit tests shortly.

[Hyperkid123]

@miha-plesko looks good.

Just FYI, we have some goodies planned that will allow you to make forms like this even easier but that is something for the future. We will get back to you once we have it prepared.

[miha-plesko]

Thanks @Hyperkid123, I've modified the component slightly to support being invoked both from:

• PhysicalServer list
• PhysicalServer details page

One thing I'm not sure is how the list page works - potentially there can be servers listed from multiple providers where each provider can have its own Toolbar override. Which button will get rendered then? 🤔 Perhaps this is more of a question for @martinpovolny who got his hands dirty with this?

Another thing I still need to figure wrt React/Enzyme is how to simulate user interaction. I would like to mount our RedfishServerProvisionDialog and select a value from the 1st drop-down and then check that 2nd drop-down gets properly populated. Do you happen to have any experience with this?

Glad to hear you're simplifying UI even further @Hyperkid123, I already like it now and you say it'll get even better so 🍻 .

[miq-bot]

Checked commits xlab-si/manageiq-providers-redfish@1f04de1~...639b15a with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
5 files checked, 1 offense detected

**

• 💣 💥 🔥 🚒 - Linter/Yaml - missing config files

[gtanzillo]

@martinpovolny Please review

[martinpovolny]

The code looks great.

One thing I'd mention is that moving forward it's preferable to use functional components not classes in React. It's where the framework is heading and in ManageIQ we are already using a pretty recent version with hooks. However using classes is not wrong, just less futureproof.

I did not test this in the UI though.

[miha-plesko]

Thanks @martinpovolny. I thought functional components can only be used in special cases where class component would only implement render; but I admit I haven't been following recent updates. In this PR we implement one functional component (PxeDetails) and one class component (RedfishServerProvisionDialog), please let me know if we have any example how to convert the class one into functional one (with componentDidMount and all) and I'll do it.

@martinpovolny may I also ask for your opinion about toolbar override? I'm worried what happens when two providers specify toolbar override for same list page. If I check two Redfish servers and two Lenovo servers on the physical servers list page -> which toolbar override will be rendered?

[Hyperkid123]

I thought functional components can only be used in special cases where class component would only implement render

There was fairly recent update to React and that added something called Hooks. That allows you to use state in functional components.

[martinpovolny]

@martinpovolny may I also ask for your opinion about toolbar override? I'm worried what happens when two providers specify toolbar override for same list page. If I check two Redfish servers and two Lenovo servers on the physical servers list page -> which toolbar override will be rendered?

The definitions are merged as hashes.

https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/helpers/application_helper/toolbar/basic.rb#L4

If they define the same keys then the behavior is "undefined" ;-) meaning the last will win, whichever is the last.

I am open to suggestions on how this can be/needs to be improved.

In the following weeks I will be doing something about this place: https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/helpers/application_helper/toolbar/basic.rb#L21

because we need to support toobar+dialog plugins that are not part of a provider and the current mechanism is based on matching the provider namespace with there @record namespace.

[miha-plesko]

@martinpovolny sorry for a bit of delay. I was afraid you're gonna say its "undefined" :) As a quick and supposedly simple solution regarding the "race condition" I'd suggest we do this:

• javascript verifies that all the selected records are of the same class
• if they aren't, the button is greyed out
• if they are, the correct implementation of the button is displayed depending on class of the selected records

This is the minimum we need to do, or else things may get dirty. For physical server button, at least, I think Lenovo will soon provide similar toolbar override like we have and then we're screwed. Or maybe not, depending if Redfish override is displayed over Lenovo one, then they're screwed :)

[miha-plesko]

@Hyperkid123 @martinpovolny can we get this PR in? All the depending PRs were merged, we're only missing this UI piece. Soon we plan on implementing two additional React modals and I'll probably try to do it "the function" way as you suggest. It if works out, I'll refactor this one then.

[martinpovolny]

I am sorry if it was not clear from my previous comment: there's no problem with this PR. Looks good to me.

I do not possess the magic green button powers on this repo.

[martinpovolny]

@miha-plesko : the overall strategy is this:

on the single-entity page, all the checking is done before the toolbar is displayed. Meaning you have a detailed control over what buttons and in what state you display.

on the listing page it's the opposite. A the moment the toolbar is calculated you do not know what entities will be selected by the user from the list. Therefor the code that runs after a button is pressed is responsible for making sure that nothing bad happens. Various things can happen (wrong type of entity selected, invalid combination of entities is selected for example). The only checks that are supported by the toolbar are about the number of items selected 1-2-many.

We where thinking at one point about providing some generic mechanism to dynamically enable/disable buttons based on entities being selected. But that is not implemented, actually even not designed and it's not a simple think to figure all the possible scenarios that we would need to adopt.

In case of Lenovo vs Redfish records selected in the listing page:
afaik no records type checking will be effective there meaning all the buttons from all plugins registered for the particular toolbar will get displayed.
That means that is't a responsibility of your (or Lenovo's) code to to check that you are running the operation on the correct type of entity.
Sorry that's all I can offer right now.

We can discuss that improvements are needed in this area and design a better behavior. But what you are suggesting:

if they are, the correct implementation of the button is displayed depending on class of the selected records

That looks to me very narrow. I am quite sure this will not match the needs of the other list-page toolbars on the application. And if we want to move forward with some checking of selected records in the list pages we should. For example in some page the criteria for a button might be if an entity is running or is retired or has something attached to it....

So if we move forward w/o some deep thought we might either implement something very specific or end up doing a server round trip to check the conditions each time a record is selected in the list. And I am not sure we are ready for that performance-wise.

Or we might need to bring some additional data with the records as they are being displayed in the grid to be able to do some fancy logic on the client.

Let's talk about this a bit more before implementing something.

btw we are working on reimplementing the Toolbars in React (now they are in AngularJS) to limit the number of technologies involved in a single page and be able to do improvements to the toolbars w/o introducing new technical debt.

[miha-plesko]

@martinpovolny thanks for explaining it out. Beware there is another issue possible: two providers can override exact same toolbar button which means javascript won't even be run to fulfil what you suggest:

That means that is't a responsibility of your (or Lenovo's) code to to check that you are running the operation on the correct type of entity.

We can workaround it by not allowing two providers to override that same specific button so we'd end up with "Lifecycle -> Provision (Redfish)" and "Lifecycle -> Provision (Lenovo)" for now.

But I fully agree with you: we are resolving a very complex issue here mostly because we're not sure what we desire. Have to be careful not to break the MIQ's "single pane of glass" paradigm too much while still allowing per-provider customization. If you can, please include me in the discussion when you'll be working on this, I'm curious what solution turns out best eventually. Using supports mixin sounds quite good so far.

[miha-plesko]

Just read your comment again and realized you're moving angular -> react and not vice versa (as I initially understood), which is what I really really like :)