fixes #23369 - load vmware datastores async #6103

timogoebel · 2018-10-03T10:10:33Z

This commit introduces async loading of vSphere Datastores and Storage Pods.

This speeds up the time before the form is required and is a prerequisite for #3559, a feature requested in 2013 that we still haven't delivered.

@ohadlevy: Do you mind reviewing this as this is react code?

theforeman-bot · 2018-10-03T10:10:47Z

Issues: #23369

amirfefer

Thanks @timogoebel 👍
I left few inline comments about the javascript part :)

amirfefer · 2018-10-03T10:27:54Z

webpack/assets/javascripts/react_app/components/common/forms/Select.js

+    const { allowClear } = this.props;
+
+    if ($.fn.select2) {
+      $(this.refs.select)


react 16.3 uses createRef API instead
Would you mind to use it?
https://reactjs.org/docs/refs-and-the-dom.html

amirfefer · 2018-10-03T10:59:44Z

webpack/assets/javascripts/react_app/redux/reducers/hosts/storage/vmware.js

+        .set('datastoresError', undefined)
+        .set('storagePods', {})
+        .set('storagePodsLoading', true)
+        .set('storagePodsError', undefined)


instead of a lot of nested .set you can use merge

amirfefer · 2018-10-03T11:05:06Z

webpack/assets/javascripts/react_app/redux/actions/hosts/storage/vmware.js

+  });
+};
+
+export const fetchDatastores = url => (dispatch) => {


should it be exported while it's used only in this file?

on the other hand, i tend to prefer to call this action from the component's componentDidMount

amirfefer · 2018-10-03T11:05:19Z

webpack/assets/javascripts/react_app/redux/actions/hosts/storage/vmware.js

+  });
+};
+
+export const fetchStoragePods = url => (dispatch) => {


amirfefer · 2018-10-03T11:14:37Z

webpack/assets/javascripts/react_app/components/hosts/storage/vmware/controller/index.js

@@ -17,6 +17,12 @@ const Controller = ({
  controllerVolumes,
  removeController,
  config,
+  datastores,


Could you add Proptype? It would be much easier to understand the component's props with it

timogoebel · 2018-10-03T11:44:19Z

@amirfefer: Thanks for the review, will take a look at your invaluable comments (and probably learn a lot!). 👍

timogoebel · 2018-10-03T13:40:27Z

@amirfefer: I hope I addressed most of your comments. Unfortunately, I did not get createRef working. I guess there is enough refactoring in this PR already, we can always change that later.

ares · 2018-10-03T21:08:02Z

I've tested the functionality and works great with and without cache enabled. I defer to @amirfefer for the createRef pattern, but 👍 form me for manual testing. I haven't really read the code. Test failures are unrelated.

ohadlevy · 2018-10-04T06:13:55Z

webpack/assets/javascripts/react_app/redux/reducers/hosts/storage/vmware.js

+      return state
+        .set('datastoresError', undefined)
+        .set('datastores', {})
+        .set('datastoresLoading', true);


please use merge instead of multiple sets, its suppose to be faster :)

ohadlevy

could you please also have a look at the storybook? I think it needs an update after these changes.

nice additions, thanks!

timogoebel · 2018-10-04T06:31:24Z

@ohadlevy: Sure, but after 20 minutes trying to get the storybook server running I gave up. Does it work for you?

$ npm run storybook

> TheForemanDevDeps@1.20.0 storybook /Users/f0218175/foremandev/foreman
> start-storybook -p 6006

@storybook/react v3.2.13

=> Loading custom .babelrc
=> Loading custom addons config.
=> Loading custom webpack config (full-control mode).
/Users/f0218175/foremandev/foreman/.storybook/webpack.config.js:7
  defaultConfig.module.rules = [
                ^

TypeError: Cannot read property 'module' of undefined
    at module.exports (/Users/f0218175/foremandev/foreman/.storybook/webpack.config.js:7:17)
    at exports.default (/Users/f0218175/foremandev/foreman/node_modules/@storybook/react/dist/server/config.js:62:12)
    at exports.default (/Users/f0218175/foremandev/foreman/node_modules/@storybook/react/dist/server/middleware.js:19:37)
    at Object.<anonymous> (/Users/f0218175/foremandev/foreman/node_modules/@storybook/react/dist/server/index.js:153:34)
    at Module._compile (module.js:660:30)
    at Object.Module._extensions..js (module.js:671:10)
    at Module.load (module.js:573:32)
    at tryModuleLoad (module.js:513:12)
    at Function.Module._load (module.js:505:3)
    at Module.require (module.js:604:17)

ohadlevy · 2018-10-04T06:35:28Z

@timogoebel it seems to work for me:

Switched to a new branch '6103'
➜  foreman git:(6103) npm run storybook                                                                                                                     git:(6103|) 

> TheForemanDevDeps@1.20.0 storybook /home/ohad/git/foreman
> start-storybook -p 6006

info @storybook/react v3.4.8
info 
info => Loading custom .babelrc
info => Loading custom addons config.
info => Loading custom webpack config (full-control mode).
 66% building modules 623/657 modules 34 active ...oreman/node_modules/lodash/isArray.jsDEPRECATION WARNING on line 1, column 8 of stdin:
Including .css files with @import is non-standard behaviour which will be removed in future versions of LibSass.
Use a custom importer to maintain this behaviour. Check your implementations documentation on how to create a custom importer.

webpack built d4fc339334bb701f4e07 in 13336ms                                             
info Storybook started on => http://localhost:6006/

ares · 2018-10-04T07:24:21Z

I have the similar issue without this PR, the npm command starts the server but navigating to storybook, I see Cannot read property 'defaults' of undefined

amirfefer

Thanks @timogoebel 👍
just few minor issues with PropType

amirfefer · 2018-10-04T12:09:01Z

webpack/assets/javascripts/react_app/components/hosts/storage/vmware/index.js

+  config: PropTypes.object,
+  controllers: PropTypes.array,
+  data: PropTypes.object,
+  datastores: PropTypes.object,


PropTypes.object is not informative
Proptype.shape is much better for arrays and objects.
Plus, if a prop is required, add isRequired to it
i.e:

PropTypes.shape({ color: PropTypes.string, fontSize: PropTypes.number }).isRequired, // if a prop is required, add `.isRequired`, else just add a default value

amirfefer · 2018-10-04T12:11:16Z

webpack/assets/javascripts/react_app/components/hosts/storage/vmware/index.js

 const mapDispatchToProps = (state) => {
-  const { controllers, config, volumes = [] } = state.hosts.storage.vmware;
+  const {
+    controllers, config, volumes = [], datastores, datastoresLoading, datastoresError,


you can use default values with Proptype:

StorageContainer.defaultProps = { volumes: [] };

ohadlevy · 2018-10-08T12:31:49Z

what is the status of this PR? (if its ready for today's cut of date?)

timogoebel · 2018-10-08T13:55:42Z

@ohadlevy: I won't be able to look at this today. Let's to it in the 1.21 cycle.

timogoebel · 2018-10-15T20:20:34Z

@amirfefer, @ohadlevy, @ares: Improved the prop types and fixed up storybook. Can you please re-review?

amirfefer · 2018-10-21T13:29:13Z

LGFM 👍

timogoebel · 2018-11-05T14:44:16Z

This is my final attempt to get this in, I rebased the code and added support for statistics of the datastore and storage pods.

ohadlevy

LGTM, I don't have a working VMWare env to test.

lizagilman · 2018-11-05T15:01:23Z

@timogoebel or anyone interested, regarding storybook issue (if you still have it), this workaround works for me - https://gist.github.com/lizagilman/d3a94114fc3fd3214138ec120fca4a5c

timogoebel · 2018-11-05T15:06:32Z

@lizagilman: Thanks. I chose the "reboot" approach. That was quite popular in ancient times.
rm -rf node_module && npm install.

timogoebel · 2018-11-05T15:07:21Z

@chris1984: Can you do me a favor and test and ack this? Thanks a bunch.

amirfefer · 2018-11-05T15:54:02Z

I've test it with vmware env and it works for me:

Looks like a snapshot needs an update

chris1984

ACK, looks good Thanks @timogoebel

chris1984 · 2018-11-05T17:40:39Z

[test foreman]

ares · 2018-11-06T07:10:09Z

It seems like test failures are related, mind to take a look?

timogoebel · 2018-11-07T08:43:58Z

Fixed the tests.

timogoebel · 2018-11-07T11:31:38Z

Tests passed.

ohadlevy · 2018-11-07T11:35:35Z

package.json

@@ -83,6 +83,7 @@
    "jstz": "~1.0.7",
    "lodash": "^4.17.10",
    "multiselect": "~0.9.12",
+    "number_helpers": "^0.1.1",


I tried packaging this, but it fails, I assume before merge we would need a packaging ACK.

EDIT: for completion, the error is

./add_npm_package.sh number_helpers 0.1.1 Found 0 dependencies - using single strategy Making directory...FINISHED Creating specs and downloading sources...---- npm2rpm ---- -_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_- - Starting npm module download: https://registry.npmjs.org/number_helpers/-/number_helpers-0.1.1.tgz - Unpacking in /tmp/npm2rpm-15831E09pZQE4YSYD ... - Finished extracting for number_helpers - Reading package.json for number_helpers Warning: No README data Warning: No license field. - Finished reading package.json for number_helpers fs.js:133 throw new ERR_INVALID_CALLBACK(); ^ TypeError [ERR_INVALID_CALLBACK]: Callback must be a function at maybeCallback (fs.js:133:9) at Object.writeFile (fs.js:1139:14) at writeSpecFile (/home/ohad/git/foreman-packaging/node_modules/npm2rpm/bin/npm2rpm.js:87:6) at Extract.tar_extract.stream.on (/home/ohad/git/foreman-packaging/node_modules/npm2rpm/bin/npm2rpm.js:78:5) at Extract.emit (events.js:187:15) at DirWriter.<anonymous> (/home/ohad/git/foreman-packaging/node_modules/tar/lib/extract.js:81:8) at DirWriter.emit (events.js:187:15) at end (/home/ohad/git/foreman-packaging/node_modules/fstream/lib/writer.js:324:14) at /home/ohad/git/foreman-packaging/node_modules/fstream/lib/writer.js:314:34 at endUtimes (/home/ohad/git/foreman-packaging/node_modules/fstream/lib/writer.js:238:46)

I think that was a warning for a while already but likely that there's an update which makes it strict. I'll have a look.

also TODO: add this package to webpack vendor file (can be done in a follow up pr).

theforeman/foreman-packaging#3174. Luckily in theforeman/npm2rpm#48 I already fixed that issue.

@ekohl great, once you are ready please ack this PR.

ekohl

Packaging ACK, haven't looked at the code :)

ohadlevy · 2018-11-07T15:27:09Z

thanks @timogoebel!

timogoebel · 2018-11-07T16:23:01Z

Thanks, @ohadlevy + @amirfefer + @ares + @ekohl + @chris1984!

timogoebel added the UI label Oct 3, 2018

theforeman-bot added Needs testing Not yet reviewed labels Oct 3, 2018

This was referenced Oct 3, 2018

fixes #23369 - load vmware datastores async #5483

Closed

fixes #2113 - vmware: filter datastores and networks based on cluster #3559

Closed

amirfefer suggested changes Oct 3, 2018

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Oct 3, 2018

timogoebel force-pushed the 23369-vmware-datastores-async-react-2 branch from caeac4d to a24ce0f Compare October 3, 2018 13:38

timogoebel added Needs re-review and removed Waiting on contributor labels Oct 3, 2018

ohadlevy reviewed Oct 4, 2018

View reviewed changes

amirfefer suggested changes Oct 4, 2018

View reviewed changes

theforeman-bot added Waiting on contributor and removed Needs re-review labels Oct 4, 2018

timogoebel force-pushed the 23369-vmware-datastores-async-react-2 branch from a24ce0f to 41d6490 Compare October 15, 2018 18:53

theforeman-bot added Needs re-review and removed Waiting on contributor labels Oct 15, 2018

timogoebel force-pushed the 23369-vmware-datastores-async-react-2 branch from 41d6490 to b5f603e Compare October 15, 2018 20:19

timogoebel dismissed amirfefer’s stale review via c554d37 November 5, 2018 14:40

timogoebel force-pushed the 23369-vmware-datastores-async-react-2 branch 2 times, most recently from c554d37 to ad0e808 Compare November 5, 2018 14:43

timogoebel added the Needs re-review label Nov 5, 2018

ohadlevy previously approved these changes Nov 5, 2018

View reviewed changes

theforeman-bot removed the Needs re-review label Nov 5, 2018

amirfefer previously approved these changes Nov 5, 2018

View reviewed changes

chris1984 previously approved these changes Nov 5, 2018

View reviewed changes

fixes #23369 - load vmware datastores async

6fa1416

timogoebel dismissed stale reviews from chris1984, amirfefer, and ohadlevy via 6fa1416 November 7, 2018 08:43

timogoebel force-pushed the 23369-vmware-datastores-async-react-2 branch from ad0e808 to 6fa1416 Compare November 7, 2018 08:43

ohadlevy reviewed Nov 7, 2018

View reviewed changes

ekohl approved these changes Nov 7, 2018

View reviewed changes

chris1984 approved these changes Nov 7, 2018

View reviewed changes

ohadlevy merged commit b4d7f9c into theforeman:develop Nov 7, 2018

terezanovotna mentioned this pull request May 18, 2020

Skeleton loading patternfly/patternfly-design-kit#221

Closed

fixes #23369 - load vmware datastores async #6103

fixes #23369 - load vmware datastores async #6103

Conversation

timogoebel commented Oct 3, 2018 • edited Loading

theforeman-bot commented Oct 3, 2018

amirfefer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirfefer Oct 3, 2018 • edited Loading

Choose a reason for hiding this comment

amirfefer Oct 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timogoebel commented Oct 3, 2018

timogoebel commented Oct 3, 2018

ares commented Oct 3, 2018

Choose a reason for hiding this comment

ohadlevy left a comment

Choose a reason for hiding this comment

timogoebel commented Oct 4, 2018

ohadlevy commented Oct 4, 2018

ares commented Oct 4, 2018

amirfefer left a comment

Choose a reason for hiding this comment

amirfefer Oct 4, 2018 • edited Loading

Choose a reason for hiding this comment

amirfefer Oct 4, 2018 • edited Loading

Choose a reason for hiding this comment

ohadlevy commented Oct 8, 2018

timogoebel commented Oct 8, 2018

timogoebel commented Oct 15, 2018

amirfefer commented Oct 21, 2018

timogoebel commented Nov 5, 2018

ohadlevy left a comment

Choose a reason for hiding this comment

lizagilman commented Nov 5, 2018 • edited Loading

timogoebel commented Nov 5, 2018

timogoebel commented Nov 5, 2018

amirfefer commented Nov 5, 2018 • edited Loading

chris1984 left a comment

Choose a reason for hiding this comment

chris1984 commented Nov 5, 2018

ares commented Nov 6, 2018

timogoebel commented Nov 7, 2018

timogoebel commented Nov 7, 2018

ohadlevy Nov 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekohl left a comment

Choose a reason for hiding this comment

ohadlevy commented Nov 7, 2018

timogoebel commented Nov 7, 2018

timogoebel commented Oct 3, 2018 •

edited

Loading

amirfefer Oct 3, 2018 •

edited

Loading

amirfefer Oct 3, 2018 •

edited

Loading

amirfefer Oct 4, 2018 •

edited

Loading

amirfefer Oct 4, 2018 •

edited

Loading

lizagilman commented Nov 5, 2018 •

edited

Loading

amirfefer commented Nov 5, 2018 •

edited

Loading

ohadlevy Nov 7, 2018 •

edited

Loading