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

Resize disk reconfigure screen #3128

Merged
merged 1 commit into from
Feb 14, 2018
Merged

Resize disk reconfigure screen #3128

merged 1 commit into from
Feb 14, 2018

Conversation

evertmulder
Copy link
Contributor

@evertmulder evertmulder commented Dec 21, 2017

Compute > Infra > VMs, select a vm, toolbar Configuration > Reconfigure selected items

PR to support resize disk from reconfigure screen.

See: #3127
screen shot 2018-01-12 at 15 03 47
screen shot 2018-01-12 at 15 06 26
screen shot 2018-01-12 at 15 09 55
screen shot 2018-01-12 at 15 13 38
screen shot 2018-01-12 at 15 15 27

Closes #3127

@martinpovolny
Copy link
Member

Restarted travis.

@gmcculloug
Copy link
Member

@dclarizio See screenshot in PR #3127. Please assign these two PRs.

@dclarizio
Copy link

@evertmulder I'm not seeing any tests, perhaps @AparnaKarve or @himdel can recommend some.

@AparnaKarve
Copy link
Contributor

@evertmulder Can you add some screenshots related to this feature?

@himdel
Copy link
Contributor

himdel commented Jan 11, 2018

(pending core because ManageIQ/manageiq-providers-vmware#164)

@himdel
Copy link
Contributor

himdel commented Jan 11, 2018

@evertmulder You'll need to rebase - no merge commits in a PR please ;)

(This should also fix the ruby specs.)

JS specs failure looks related

vm.setEnableAddDiskButton = function () {
var nrDisksAfterReconfigure=0;
angular.forEach(vm.reconfigureModel.vmdisks, function(disk){
switch (disk.add_remove) {
Copy link
Contributor

@himdel himdel Jan 11, 2018

Choose a reason for hiding this comment

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

Wrong indentation ;)

Please see the rest of the code climate warnings - feel free to ignore "Function updateDisksAddRemove has 36 lines of code (exceeds 25 allowed). Consider refactoring." but the rest is definitely relevant :).

"placeholder" => "Enter Size",
"ng-change" => "vm.hdChanged()",
"validate-multiple" => "1",
:miqmin => "{{ (disk.orgHdUnit=='GB') && (1024*disk.orgHdSize+1) || (1*disk.orgHdSize+1)}}",
Copy link
Contributor

@himdel himdel Jan 11, 2018

Choose a reason for hiding this comment

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

CC won't warn here, but please keep the same rules...

:miqmin => "{{ (disk.orgHdUnit === 'GB') && (1024 * disk.orgHdSize + 1) || (1 * disk.orgHdSize + 1) }}",

EDIT: also, consider using ?: instead of && || unless you're really trying to convey the equivalent of
(disk.orgHdUnit === 'GB') ? ((1024 * disk.orgHdSize + 1) || (1 * disk.orgHdSize + 1)) : (1 * disk.orgHdSize + 1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@himdel Sorry, over looked this one. Should be fix now.

@himdel
Copy link
Contributor

himdel commented Jan 11, 2018

As for tests, I'm not sure what to suggest, sorry - can't really tell the refactoring bits from the actual changes (this being one big commit).

@evertmulder Care to elaborate what this actually changes please? :) (The screenshot might also help.)

@evertmulder
Copy link
Contributor Author

evertmulder commented Jan 11, 2018

@himdel Thanks for all the reviews. Tomorrow I will make some time addressing the comments. Sorry for the late response, the comments are not going to my corporate mail, so I just saw them.

@AparnaKarve Added screenshots

@evertmulder
Copy link
Contributor Author

I am afraid I did merge instead of rebase. Can I do something to fix this?

@AparnaKarve
Copy link
Contributor

Try removing those 2 merge commits 93bfb65 and ff0f048 and then a rebase

@evertmulder
Copy link
Contributor Author

evertmulder commented Jan 15, 2018

removed the merge commits. Currently working on the provider for this PR.

@agrare
Copy link
Member

agrare commented Jan 18, 2018

ManageIQ/manageiq-providers-vmware#164 has been merged

@agrare
Copy link
Member

agrare commented Jan 18, 2018

@miq-bot remove-label pending core

@martinpovolny
Copy link
Member

Can you, please, address the @miq-bot issue?

Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

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

👍 confirmed this PR worked while testing ManageIQ/manageiq-providers-vmware#164

@himdel
Copy link
Contributor

himdel commented Jan 24, 2018

JS failures still relevant,

also, please run eslint manually on your new code, seems like CC is not reporting properly... I'm seeing at least these in a few places..

  error    Expected indentation of 2 spaces but found 4  indent
  error    Expected space(s) after "if"                  keyword-spacing
  warning  Expected { after 'if' condition               curly
  warning  Expected { after 'else'                       curly

@miq-bot add_label wip

@miq-bot miq-bot changed the title Resize disk reconfigure screen [WIP] Resize disk reconfigure screen Jan 24, 2018
@miq-bot miq-bot added the wip label Jan 24, 2018
@evertmulder
Copy link
Contributor Author

evertmulder commented Jan 24, 2018

@himdel Fixed all eslint issues I could find in the file. Fixed 127 issues.
I will take a look a the jasmin issues also. Fixed the jasmin issues.
It seems all PRs fail with the JS testsuite:

submitButtonClicked when the API call fails clears flash messages

for ruby 2.4.2

I keep getting Redux errors running the test suite (like Redux API ManageIQ.redux namespace should be defined), but travis does not.

@evertmulder evertmulder changed the title [WIP] Resize disk reconfigure screen Resize disk reconfigure screen Jan 27, 2018
@miq-bot miq-bot removed the wip label Jan 27, 2018
@himdel
Copy link
Contributor

himdel commented Jan 30, 2018

It seems all PRs fail with the JS testsuite

@evertmulder Sorry about that , should be fixed now, restarting travis.

That said, can you also look at #3128 (review) please? :)

@himdel himdel closed this Jan 30, 2018
@himdel himdel reopened this Jan 30, 2018
@miq-bot
Copy link
Member

miq-bot commented Jan 31, 2018

Checked commit evertmulder@ffa6ad5 with ruby 2.3.3, rubocop 0.52.0, 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.5-compliant syntax, but you are running 2.3.3.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.

@himdel himdel closed this Feb 8, 2018
@himdel himdel reopened this Feb 8, 2018
@evertmulder
Copy link
Contributor Author

@himdel Is there anything I can improve? I think I fixed all review comments ...

@himdel
Copy link
Contributor

himdel commented Feb 12, 2018

@evertmulder Sorry about the delay, there was a spec failure but it was on our side..
Restarted, looking again.

@himdel himdel added this to the Sprint 80 Ending Feb 26, 2018 milestone Feb 14, 2018
@himdel
Copy link
Contributor

himdel commented Feb 14, 2018

LGTM, tested in the UI 👍

@himdel himdel merged commit 8835899 into ManageIQ:master Feb 14, 2018
@simaishi
Copy link
Contributor

simaishi commented Jun 1, 2018

@himdel can this be gaprindashvili/yes? We need reconfigure disk feature in G-branch (ManageIQ/manageiq#16711 (comment))

@himdel
Copy link
Contributor

himdel commented Jun 1, 2018

@simaishi Assuming ManageIQ/manageiq-providers-vmware#164 can be, I think there is no problem here.

simaishi pushed a commit that referenced this pull request Jun 1, 2018
Resize disk reconfigure screen
(cherry picked from commit 8835899)
@simaishi
Copy link
Contributor

simaishi commented Jun 1, 2018

Gaprindashvili backport details:

$ git log -1
commit c1d8f212ec734e1dc4f9ede5358d12b7128f3274
Author: Martin Hradil <himdel@seznam.cz>
Date:   Wed Feb 14 13:35:18 2018 +0000

    Merge pull request #3128 from evertmulder/ResizeDisk_ReconfigureScreen
    
    Resize disk reconfigure screen
    (cherry picked from commit 8835899188969665caca9894400db51c48dbd645)

@simaishi
Copy link
Contributor

simaishi commented Jun 1, 2018

Thanks Martin, backported VMware PR as well.

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.

10 participants