[#911] Add validation to migration throttling inputs and add CPU throttling field #915

mzazrivec · 2019-03-19T15:46:49Z

Initial screen

Validation error

Validation pass

Fixes: #911

Associated RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1690851

codeclimate · 2019-03-19T15:47:23Z

app/javascript/react/screens/App/Settings/screens/GeneralSettings/GeneralSettings.js

@@ -34,14 +34,30 @@ export class GeneralSettings extends React.Component {
  };

  render() {
-    const { isFetchingServers, isFetchingSettings, isSavingSettings, savedSettings, settingsForm } = this.props;
+    const {


Similar blocks of code found in 9 locations. Consider refactoring.

codeclimate · 2019-03-19T15:47:23Z

app/javascript/react/screens/App/common/forms/TextInputWithCheckbox.js

-      input: { value, onChange },
-      postfix
-    } = this.props;
+    const { id, label, input, postfix, initialUncheckedValue, meta } = this.props;


Similar blocks of code found in 6 locations. Consider refactoring.

mturley

This looks pretty good! What happened to those postfix labels, though?

mturley · 2019-03-22T20:42:24Z

app/javascript/react/screens/App/Settings/__tests__/__snapshots__/SettingsReducer.test.js.snap

@@ -287,8 +287,10 @@ Object {
  "isSavingSettings": false,
  "postConversionHostsResults": Array [],
  "savedSettings": Object {
+    "cpu_limit_per_host": undefined,


We should add these fields to the SettingsReducer tests so they appear in the snapshots here.

You're right. Fixed.

mturley · 2019-03-22T20:44:51Z

app/javascript/react/screens/App/common/forms/TextInputWithCheckbox.js

-          </div>
+          <InputGroup>
+            <FormControl type="text" name={id} id={id} readOnly={!this.state.inputEnabled} {...input} />
+            <InputGroup.Addon>{postfix}</InputGroup.Addon>


I haven't had a chance yet to test this locally, but based on your screenshots it doesn't look like this postfix is rendering anymore?

Yup, those labels are being rendered correctly (I forgot to update screenshots in the initial comment).

mzazrivec · 2019-03-25T11:53:42Z

What happened to those postfix labels, though?

Those postfix labels do work, I just forgot to update the screenshots with the working version. Fixed now.

mturley

Forgot to get back around to this... this looks good, just waiting to merge it until after ManageIQ/manageiq#18576 is merged.

mturley · 2019-04-10T17:41:07Z

This PR is associated with this BZ targeted for 5.10.4: https://bugzilla.redhat.com/show_bug.cgi?id=1690851

mturley · 2019-04-17T18:02:00Z

@mzazrivec sorry for the additional churn, but based on the call with @fdupont-redhat earlier today, it looks like CPU throttling will make it in for 5.10.4 but the network throttling will need to wait until 5.10.5.

So... deja vu... can we comment out the network field and turn this PR into CPU throttling only, and then open another PR to re-enable the network field?

This commit can be reverted later, once the backend support is in.

miq-bot · 2019-04-18T09:47:38Z

Checked commits mzazrivec/miq_v2v_ui_plugin@a2d20cf~...929d236 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. ⭐

mzazrivec · 2019-04-18T09:48:17Z

@mturley Sure, I removed the network throttling in a separate commit, which can be later reverted (once backend support is in).

mturley · 2019-04-18T13:43:13Z

Perfect, thanks @mzazrivec. I moved the network part of the issue to #940 so we can still close #911 here.

mturley · 2019-04-18T14:18:36Z

@mzazrivec, last thing, I promise.... I was giving your changes a final review and I noticed we never right-aligned the field names for the Concurrent Migrations section like they are in the mockups. We should have those two field labels right-aligned, and the ones with the checkboxes left-aligned. If you could make that change I'll approve the PR, sorry I didn't notice sooner.

mzazrivec · 2019-04-18T14:31:59Z

@mturley Initially, I did try to implement it that way, but it did not look very good in my opinion:

vs. all labeles left aligned:

I can make the change, if you insist, but I'd rather keep it the way it looks right now, since mixing the label alignments produces a bit funny look & fell (in my opinion of course).

mturley · 2019-04-18T14:59:21Z

I think I agree with you, but I'll let @vconzola make the final decision.

vconzola · 2019-04-18T15:19:53Z

Per our discussion in the v2v standup meeting, @mturley will try to make a simple change to the CSS class to use top aligned labels. But if it's too much, let's just leave them left aligned.

mturley · 2019-04-18T16:01:37Z

@mzazrivec here's a commit that switches the form to a vertical layout: mturley@ef218c0
(edit: replaced 100px field width with 150px so the validation message fits)
(edit 2: fixed some padding in response to feedback from Vince)

It looks like this. @vconzola does this look good to you?

With validation error showing:

vconzola · 2019-04-18T17:00:21Z

Other than my few comments in Slack, LGTM. Ship it.

mturley · 2019-04-18T17:16:11Z

Ok @mzazrivec I made a couple other tweaks to the commit based on feedback from Vince, here's the final version to cherry-pick: mturley@ef218c0

mturley · 2019-04-18T18:42:21Z

The backend PR this depended on is also now merged (ManageIQ/manageiq#18576).

mturley · 2019-04-18T18:54:51Z

I'm going to go ahead and merge this one as-is so it can make the GA build, I'll open a separate PR to change the layout to vertical.

…rottling_inputs [#911] Add validation to migration throttling inputs and add CPU throttling field (cherry picked from commit 03106c9) https://bugzilla.redhat.com/show_bug.cgi?id=1702085

simaishi · 2019-04-22T22:29:26Z

Hammer backport details:

$ git log -1
commit ea0aa2770cedba6d7c4098891e79d64244945652
Author: Mike Turley <[email protected]>
Date:   Thu Apr 18 14:56:43 2019 -0400

    Merge pull request #915 from mzazrivec/add_validation_to_migration_throttling_inputs
    
    [#911] Add validation to migration throttling inputs and add CPU throttling field
    
    (cherry picked from commit 03106c9ca7a9c72d54e70c1eddfac9af53f3404f)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1702085

…ation_throttling_inputs" This reverts commit ea0aa27.

simaishi · 2019-05-07T22:21:20Z

Reverted Hammer backport.

commit 0fed6ddaf19579f0c5cdec13a7e7a2b704bfc66f
Author: Satoe Imaishi <[email protected]>
Date:   Tue May 7 18:17:42 2019 -0400

    Revert "Merge pull request #915 from mzazrivec/add_validation_to_migration_throttling_inputs"
    
    This reverts commit ea0aa2770cedba6d7c4098891e79d64244945652.

mzazrivec added the enhancement label Mar 19, 2019

codeclimate bot reviewed Mar 19, 2019

View reviewed changes

mzazrivec changed the title ~~Add validation to migration throttling inputs~~ [WIP] Add validation to migration throttling inputs Mar 19, 2019

mzazrivec added the wip label Mar 19, 2019

mzazrivec force-pushed the add_validation_to_migration_throttling_inputs branch from c934b51 to c864c98 Compare March 19, 2019 15:58

mzazrivec changed the title ~~[WIP] Add validation to migration throttling inputs~~ Add validation to migration throttling inputs Mar 19, 2019

mzazrivec removed the wip label Mar 19, 2019

mzazrivec force-pushed the add_validation_to_migration_throttling_inputs branch from ca776f7 to c8c6166 Compare March 20, 2019 10:01

mturley reviewed Mar 22, 2019

View reviewed changes

mzazrivec force-pushed the add_validation_to_migration_throttling_inputs branch from c8c6166 to 16c1b4a Compare March 25, 2019 12:05

mturley added hammer/yes z-stream v1.2 labels Mar 25, 2019

mturley self-assigned this Mar 25, 2019

mzazrivec changed the title ~~Add validation to migration throttling inputs~~ [#911] Add validation to migration throttling inputs Mar 26, 2019

mturley reviewed Apr 4, 2019

View reviewed changes

mturley changed the title ~~[#911] Add validation to migration throttling inputs~~ [#911] Add validation to migration throttling inputs and add CPU and network throttling fields Apr 10, 2019

mturley mentioned this pull request Apr 16, 2019

[V2V] Add CPU and network throttling in model ManageIQ/manageiq#18576

Merged

mzazrivec added 3 commits April 18, 2019 11:26

Add validation to migration throttling inputs

a2d20cf

Update tests for new migration throttling inputs

a7343ca

Temporarily remove network_limit_per_host

929d236

This commit can be reverted later, once the backend support is in.

mzazrivec force-pushed the add_validation_to_migration_throttling_inputs branch from 16c1b4a to 929d236 Compare April 18, 2019 09:47

mturley changed the title ~~[#911] Add validation to migration throttling inputs and add CPU and network throttling fields~~ [#911] Add validation to migration throttling inputs and add CPU throttling field Apr 18, 2019

mturley mentioned this pull request Apr 18, 2019

Add/un-hide throttling setting for network limits #940

Closed

mturley approved these changes Apr 18, 2019

View reviewed changes

mturley merged commit 03106c9 into ManageIQ:master Apr 18, 2019

This was referenced Apr 18, 2019

Change layout of throttling settings form from horizontal to vertical #943

Merged

[Changelog] Update for Hammer-5 release #939

Merged

simaishi added hammer/backported and removed hammer/yes labels Apr 22, 2019

simaishi added a commit that referenced this pull request May 7, 2019

Revert "Merge pull request #915 from mzazrivec/add_validation_to_migr…

0fed6dd

…ation_throttling_inputs" This reverts commit ea0aa27.

simaishi added hammer/yes and removed hammer/backported labels May 7, 2019

Fryguy added hammer/yes? and removed hammer/yes labels Nov 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#911] Add validation to migration throttling inputs and add CPU throttling field #915

[#911] Add validation to migration throttling inputs and add CPU throttling field #915

mzazrivec commented Mar 19, 2019 •

edited by mturley

Loading

codeclimate bot Mar 19, 2019

codeclimate bot Mar 19, 2019

mturley left a comment

mturley Mar 22, 2019

mzazrivec Mar 25, 2019

mturley Mar 22, 2019

mzazrivec Mar 25, 2019

mzazrivec commented Mar 25, 2019

mturley left a comment

mturley commented Apr 10, 2019

mturley commented Apr 17, 2019

miq-bot commented Apr 18, 2019

mzazrivec commented Apr 18, 2019

mturley commented Apr 18, 2019

mturley commented Apr 18, 2019

mzazrivec commented Apr 18, 2019

mturley commented Apr 18, 2019

vconzola commented Apr 18, 2019

mturley commented Apr 18, 2019 •

edited

Loading

vconzola commented Apr 18, 2019

mturley commented Apr 18, 2019 •

edited

Loading

mturley commented Apr 18, 2019

mturley commented Apr 18, 2019

simaishi commented Apr 22, 2019

simaishi commented May 7, 2019 •

edited

Loading

[#911] Add validation to migration throttling inputs and add CPU throttling field #915

[#911] Add validation to migration throttling inputs and add CPU throttling field #915

Conversation

mzazrivec commented Mar 19, 2019 • edited by mturley Loading

codeclimate bot Mar 19, 2019

Choose a reason for hiding this comment

codeclimate bot Mar 19, 2019

Choose a reason for hiding this comment

mturley left a comment

Choose a reason for hiding this comment

mturley Mar 22, 2019

Choose a reason for hiding this comment

mzazrivec Mar 25, 2019

Choose a reason for hiding this comment

mturley Mar 22, 2019

Choose a reason for hiding this comment

mzazrivec Mar 25, 2019

Choose a reason for hiding this comment

mzazrivec commented Mar 25, 2019

mturley left a comment

Choose a reason for hiding this comment

mturley commented Apr 10, 2019

mturley commented Apr 17, 2019

miq-bot commented Apr 18, 2019

mzazrivec commented Apr 18, 2019

mturley commented Apr 18, 2019

mturley commented Apr 18, 2019

mzazrivec commented Apr 18, 2019

mturley commented Apr 18, 2019

vconzola commented Apr 18, 2019

mturley commented Apr 18, 2019 • edited Loading

vconzola commented Apr 18, 2019

mturley commented Apr 18, 2019 • edited Loading

mturley commented Apr 18, 2019

mturley commented Apr 18, 2019

simaishi commented Apr 22, 2019

simaishi commented May 7, 2019 • edited Loading

mzazrivec commented Mar 19, 2019 •

edited by mturley

Loading

mturley commented Apr 18, 2019 •

edited

Loading

mturley commented Apr 18, 2019 •

edited

Loading

simaishi commented May 7, 2019 •

edited

Loading