-
Notifications
You must be signed in to change notification settings - Fork 999
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
Fixes #4509 - VMWare scsi react #4366
Conversation
@matanwerbner, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ohadlevy, @gailsteiger and @timogoebel to be potential reviewers. |
webpack/stories/index.js
Outdated
|
||
function hide() { | ||
show = false; | ||
storiesOf('Power Status', module).add('Loading', () => (<PowerStatusInner/>)).add('ON', () => (<PowerStatusInner state="on" title="on" statusText="On"/>)).add('OFF', () => (<PowerStatusInner state="off" title="off" statusText="Off"/>)).add('N/A', () => (<PowerStatusInner state="na" statusText="No power support" title="N/A"/>)).add('Error', () => (<PowerStatusInner |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 81 exceeds the maximum line length of 100 max-len
app/controllers/hosts_controller.rb
Outdated
deep_symbolize_keys | ||
volumes = {} | ||
scsi_and_vol[:volumes].each_with_index do |vol, index| | ||
volumes["#{index}"] = vol |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer to_s over string interpolation.
03895da
to
cfc15f3
Compare
config/webpack.config.js
Outdated
@@ -100,7 +100,7 @@ if (production) { | |||
hot: true | |||
}; | |||
// Source maps | |||
config.devtool = 'inline-source-map'; | |||
config.devtool = 'eval'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not related to this pr?
@@ -0,0 +1 @@ | |||
$light-grey: #efefef; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this is from patternfly pallet? https://www.patternfly.org/styles/color-palette/#_
|
||
const CommonForm = (props) => { | ||
return ( | ||
<div className={ `form-group ${props.className || ''}` }> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cant we default the props in the const, e.g.:
const CommonForm = ({className = '', label = ''}) => { ...
}; | ||
|
||
Select.defaultProps = { | ||
className: '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inconsistent with CommonForm?
@@ -0,0 +1,11 @@ | |||
.host-power-status { | |||
&.on { | |||
color: #3f9c35; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not related to the pr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i've moved powerStatus files to their own dedicated directory inside hosts, thats the relation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH: based on the size of this PR, please do the restructuring in another PR that we most likely can merge quickly, then you can rebase this pr again.
value={size} | ||
className="text-vmware-size" | ||
onChange={updateDisk.bind(this, 'size')} | ||
label={__('Size (GB)')} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we include any basic validation (e.g. size is bigger than 0 and and is integer?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matanwerbner I'm OK with doing it in a separate PR, please open an issue for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updateController({[attribute]: value}); | ||
}; | ||
|
||
const _updateDisk = (diskIndex, attribute, e) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whats the difference for this function? comment maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they each dispatch a different action -
one handles changes for the controller object,
the other for disk object.
storagePod: '', | ||
thinProvision: false, | ||
eagerZero: false, | ||
name: 'Hard disk' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be translated?
@@ -14,3 +14,9 @@ export default createStore( | |||
window.__REDUX_DEVTOOLS_EXTENSION__ && window.__REDUX_DEVTOOLS_EXTENSION__(), | |||
applyMiddleware(...middleware) | |||
); | |||
|
|||
export const getNewStore = () => createStore( | |||
reducer, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does this do? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this creates a new store, it is very usefull for testing
@@ -1,7 +1,5 @@ | |||
import Immutable from 'seamless-immutable'; | |||
export const initialState = Immutable({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please elaborate on all the structure changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
state is changed to { hosts: { powerStatus: {}, storage: {} } }
export const MaxDisksPerController = 15; | ||
export const MaxControllers = 4; | ||
export const ControllerTypes = { | ||
VirtualBusLogicController: 'Bus Logic Parallel', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get these values from the rails model?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, can you help put those there? my ruby is very weak
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matanwerbner : Thanks for taking over the PR! I did not test provisioning, yet. But I have some comments regarding the react UI.
className="cb-controller-is-selected" | ||
checked={controller.isSelected} | ||
onChange={_updateController.bind(this, 'isSelected')}/> | ||
<label>{__('Create SCSI controller')}</label> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This checkbox should be checked by default. A VM needs at least one scsi controller or your VM won't have any storage.
I'm wondering if this checkbox is actually useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is by design supplied (i will attach), it's only purpose is to decide which controllers get deleted when you click "delete controllers".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the design is flawed then. It would be better to have a dedicated delete button per controller (we already have a dedicated button per disk). The checkbox is very confusing. I didn't get, that it's related to the Delete Controller
button.
btw: This is how this looks in VMWare. The UI is pretty bad I think, but Foreman users might be familiar with this:
(Sorry for the German language, but I guess you get the point)
@Rohoover: Do you agree? Any more comments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original design was provided here: #3979 (comment)
It was done with a low level of effort in mind. The checkboxes work similar to the Foreman tables, where items are selected before actions occur to them.
That said, alternative layouts can be done. Attached is another quick mockup with you feedback. I think it we wanted we can also add the chevrons (accordion) feature shown in the VMWare UI as well if desired.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Rohoover : Thanks. I think with this design it's a lot more clear how to delete a controller. I like that.
I do think the chevrons are a good idea. However I don't think it's a requirement for this PR.
@matanwerbner: If you want to earn some extra points, go ahead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'll try to get the chevrons in soon, i have some things on my plate prior to that, so for now i just removed the checkboxes...
value={props.value} | ||
onChange={props.onChange} | ||
> | ||
<option value="">{__('Please select')}</option> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we hide this if a user selects something, e.g. make this a real placeholder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is hidden when a user selects another options, it is regular select behavior, same as other options in the select
<Select | ||
label={__('Data store')} | ||
value={dataStore} | ||
onChange={updateDisk.bind(this, 'dataStore')} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The data doesn't update when you change the compute resource. We have a couple of VMWare compute resources that all have different datastores.
If I select one compute resource, then another one it still shows the datastores of the first one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition if you create a new volume, the select fields aren't rendered as `select2´. The usage makes a lot of sense, though. We have at least 100 datastores. Select2 helps a lot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
first comment: the json seems to update OK, can you check if maybe it gets lost in the server somehow?
as for select2 - i will add for compute resource
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matanwerbner I think this is an issue as the react component is already initialized, it wont be mounted again.
afaiu steps to reproduce are:
- have two different vmware compute resource
- create a new host, select the first vmware, then change the compute resource to the second vmware
first vmware form will persist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@timogoebel i believe i've solved this issue, can you pull and check again?
|
||
return ( | ||
<div className="disk-container"> | ||
<TextInput |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be read-only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
disk name should be read only? maybe we should no show it at all then..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the name is generated by VMWare. A user cannot change it but should be able to see it when editing a host (or he would edit the wrong disk by accident).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currently they will all show "hard disk" since it is taken from consts. should i implement an ajax request to fetch the new volume name from the server?
}); | ||
|
||
return ( | ||
<div className="disk-container"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disk mode parameter is missing. It was introduced in 1d5f6c2.
cfc15f3
to
f91c4d9
Compare
expect( | ||
wrapper.render().find('.controller-container').length | ||
).toEqual(1); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing spaces not allowed no-trailing-spaces
f91c4d9
to
8a7a316
Compare
@@ -0,0 +1,14 @@ | |||
export const defaultConrollerAttributes = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo in filename, it should be vmware.const.js
@@ -7,7 +7,12 @@ export const ACTIONS = { | |||
NOTIFICATIONS_EXPAND_DRAWER_TAB: 'NOTIFICATIONS_EXPAND_DRAWER_TAB', | |||
NOTIFICATIONS_MARK_AS_READ: 'NOTIFICATIONS_MARK_AS_READ', | |||
NOTIFICATIONS_MARKED_AS_READ: 'NOTIFICATIONS_MARKED_AS_READ', | |||
NOTIFICATIONS_SET_REQUEST_STATUS: 'NOTIFICATIONS_SET_REQUEST_STATUS' | |||
NOTIFICATIONS_SET_REQUEST_STATUS: 'NOTIFICATIONS_SET_REQUEST_STATUS', | |||
CONTROLLER_ADDED: 'CONTROLLER_ADDED', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whats the difference to the STORAGE_VMWARE_ADD_CONTROLLER
const?
2b91ef6
to
ba7f287
Compare
@matanwerbner : I'm super busy this week. Sorry for the delay. I do want this feature, appreciate your work and will gladly help you. I'll check this out first thing next week. Please bear with me here. |
ba7f287
to
786ff5e
Compare
app/controllers/hosts_controller.rb
Outdated
@@ -925,4 +926,18 @@ def host_attributes_for_templates(host) | |||
def csv_columns | |||
[:name, :operatingsystem, :environment, :model, :hostgroup, :last_report] | |||
end | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing whitespace detected.
f410f39
to
46ae0af
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matanwerbner : Sorry for the delay in reviewing this. All in all, it looks good, but I found some issue while testing this.
First of all I added code to pass the controller_types to react:
timogoebel@c047ccc
Just squash that in if you want.
Second of all, the code does not pass the values of datastore
or storage_pod
in the JSON hash send via post. The fields are just empty strings. That's why vm creation fails right now.
When there are errors creating the VM, the react part of the form is reset.
When you edit a vm, it does not pick up the present values. The form should show the current data from the VM, e.g. if the VM has three volumes it should show three volumes. All fields except the size should be disabled as fog-vsphere (the library we use for vmware management) does not support editing any more parameters of a volume.
In addition, I left some comments inline.
And thanks again for doing this, first react form and a vmware form - that's not trivial 👍
<TextInput | ||
value={name} | ||
onChange={updateDisk.bind(this, 'name')} | ||
label={__('Disk name')} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be read-only or not even a text field at all. The name is interesting for users but can't actually be changed at all. Please see #4252.
const selectablePods = renderOptions(storagePods); | ||
|
||
return ( | ||
<div className="disk-container"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disk mode select box is missing which was recently added. Please see #4314
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added, do you want to send the possible values the same was as controller_types?
/> | ||
|
||
<Select | ||
label={__('Storage Pod')} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we hide this if there aren't any storage pods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And if you select a storage pod we have to disable or hide the datastore select.
You can create a VM either on a storage pod or on a datastore. Storage pod has the higher priority. If a user selects a storage pod, vmware host creation will ignore what was selected as the datastore.
<label>{__('Create SCSI controller')}</label> | ||
</div> | ||
<div className="controller-type-container col-md-4"> | ||
<select |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a select2 as well when adding a new controller.
@timogoebel I've taken care of most of what you've asked, just a couple of questions: thanks |
217aaf5
to
4b5a794
Compare
Actually, we don't have that anywhere else. We always use the pattern I suggested. But maybe @Rohoover has some advice here.
I mean the blue json output under the react form. |
4b5a794
to
6c58afe
Compare
gotcha. done. |
@matanwerbner: Thanks. I retested this.
diff --git a/app/views/compute_resources_vms/form/vmware/_base.html.erb b/app/views/compute_resources_vms/form/vmware/_base.html.erb
index 7695f0e92..46cdeb12b 100644
--- a/app/views/compute_resources_vms/form/vmware/_base.html.erb
+++ b/app/views/compute_resources_vms/form/vmware/_base.html.erb
@@ -52,6 +52,6 @@ end %>
storagePods: vsphere_storage_pods(compute_resource),
datastores: vsphere_datastores(compute_resource)
},
- volumes: f.object.volumes.map { |volume| volume.attributes.deep_transform_keys { |key| key.to_s.camelize(:lower).to_sym }.reject { |k,v| k == :Size } },
+ volumes: f.object.volumes.map { |volume| volume.attributes.merge(:size_gb => volume.size_gb).deep_transform_keys { |key| key.to_s.camelize(:lower).to_sym }.reject { |k,v| k == :Size } },
controllers: f.object.scsi_controllers
}.to_json) %>
The rest works great! Good job. |
b6c83c7
to
75193d2
Compare
@timogoebel requested changes have been commited, please check |
@matanwerbner : Thanks. Nicely done. @ohadlevy made some minor changes in ohadlevy@541fb96 . Could you squash them in except for the precision / decimal stuff for sizeGb? sizeGb has to be an integer. Float does not work. Unfortunately I found another bug. When you clone a host the scsi and harddisk settings aren't cloned to the new host. :-( Here is the fix: diff --git a/app/models/compute_resources/foreman/model/vmware.rb b/app/models/compute_resources/foreman/model/vmware.rb
index 1b4fd6ee0..38817d624 100644
--- a/app/models/compute_resources/foreman/model/vmware.rb
+++ b/app/models/compute_resources/foreman/model/vmware.rb
@@ -553,6 +553,9 @@ module Foreman::Model
hsh[index.to_s] = interface_attrs
hsh
end
+ vm_attrs[:scsi_controllers] = vm.scsi_controllers.map do |controller|
+ controller.attributes
+ end
vm_attrs
end I also tested compute profiles. The volumes don't seem to get saved there. I'll investigate. |
Unfortunately, I found some more serious issues with this. Edit: Fix posted below. |
@matanwerbner I've made a few minor fixes and improvements to the storybook, can you merge ohadlevy@e36314d into this PR as well? thanks! |
@matanwerbner : I have completed my fix for the compute profiles: timogoebel@b1c47b9 This also contains a tiny test for the scsi controller normalization and @ohadlevy's review fixes. It does not contain ohadlevy/foreman@e36314d. To summarize: |
bbfe56f
to
820e916
Compare
@timogoebel i'm having difficulties and errors applying your patch, can you rebase/pull from develop? |
const { | ||
addController, | ||
controllers, | ||
volumes, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: whitespace.
return controllers.map((controller, idx) => { | ||
const controllerVolumes = volumes.filter( | ||
v => v.controllerKey === controller.key | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whitespace in here and below
820e916
to
453cd35
Compare
webpack/stories/index.js
Outdated
@@ -14,6 +14,8 @@ import PowerStatusInner | |||
from '../assets/javascripts/react_app/components/hosts/powerStatus/powerStatusInner'; | |||
import Toast | |||
from '../assets/javascripts/react_app/components/toastNotifications/toastListitem'; | |||
import StorageContainer from '../assets/javascripts/react_app/components/hosts/storage/vmware'; | |||
import * as VMWareData from './data/storage/vmware'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'VMWareData' is defined but never used no-unused-vars
webpack/stories/index.js
Outdated
@@ -14,6 +14,8 @@ import PowerStatusInner | |||
from '../assets/javascripts/react_app/components/hosts/powerStatus/powerStatusInner'; | |||
import Toast | |||
from '../assets/javascripts/react_app/components/toastNotifications/toastListitem'; | |||
import StorageContainer from '../assets/javascripts/react_app/components/hosts/storage/vmware'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'StorageContainer' is defined but never used no-unused-vars
453cd35
to
a482a57
Compare
a482a57
to
afe0c18
Compare
replaced by #4676 - thanks @matanwerbner :-) |
Adding vmware storage controllers + volumes in react
@timogoebel can you please check it's working with server side? ty