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.

Sign up for GitHub

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

Jump to bottom

Dealing with disks which have no controller key #77

Merged
merged 1 commit into from
Aug 1, 2017
Merged

Dealing with disks which have no controller key #77

merged 1 commit into from
Aug 1, 2017

Conversation

jameswnl
Copy link
Contributor

@jameswnl jameswnl commented Jul 18, 2017 •
edited
Loading

@Fryguy
Copy link
Member

Fryguy commented Jul 20, 2017

I was talking with @agrare here and I think it's preferable to actually collect the disk, but set {:controller_type => "unknown", :location => nil} instead. This allows the disks to still appear in the UI, and the choice of unknown/nil is intentional because it presents nicely in the UI.

agrare
agrare requested changes Jul 31, 2017
View reviewed changes
app/models/manageiq/providers/vmware/infra_manager/refresh_parser.rb Outdated

storage_mor = backing['datastore']

new_result = {
:device_name => device.fetch_path('deviceInfo', 'label'),
:device_type => device_type,
:controller_type => controller_type,
:controller_type => controller_type || 'unknown',
Copy link
Member

Choose a reason for hiding this comment

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

Since you already have if controller else 'unknown' is the || 'unknown' necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

haha, yeah, WIP. Now is done. Thanks

Copy link
Member

Choose a reason for hiding this comment

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

Hey I still think you only need the 'unknown' in one place

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i can only ... 😭

done now. also updated the location to nil (instead of '') as suggested by Jason

app/models/manageiq/providers/vmware/infra_manager/refresh_parser.rb Outdated
:present => true,
:filename => backing['fileName'] || backing['deviceName'],
:location => "#{controller['busNumber']}:#{device['unitNumber']}",
:location => controller ? "#{controller['busNumber']}:#{device['unitNumber']}": ''
Copy link
Member

Choose a reason for hiding this comment

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

Can you pull this into a location = if controller ... like what you have for controller_type?

@jameswnl jameswnl changed the title [WIP] skip disks which have no controller key Dealing with disks which have no controller key Jul 31, 2017
@jameswnl
Copy link
Contributor Author

@miq-bot remove_label wip

@miq-bot miq-bot removed the wip label Jul 31, 2017
agrare
agrare requested changes Aug 1, 2017
View reviewed changes
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.

Just that one nit then this looks good :)

app/models/manageiq/providers/vmware/infra_manager/refresh_parser.rb Outdated

storage_mor = backing['datastore']

new_result = {
:device_name => device.fetch_path('deviceInfo', 'label'),
:device_type => device_type,
:controller_type => controller_type,
:controller_type => controller_type || 'unknown',
Copy link
Member

Choose a reason for hiding this comment

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

Hey I still think you only need the 'unknown' in one place

spec/models/manageiq/providers/vmware/infra_manager/refresher_spec.rb
:disk_type => "thin",
:start_connected => true
)

Copy link
Member

Choose a reason for hiding this comment

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

Huge 👍 for the test 😄

@miq-bot
Copy link
Member

miq-bot commented Aug 1, 2017

Checked commit https://github.com/jameswnl/manageiq-providers-vmware/commit/b5f1089beb71f98026f22e8352383ecf83a04593 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 1 offense detected

spec/models/manageiq/providers/vmware/infra_manager/refresher_spec.rb

agrare
agrare approved these changes Aug 1, 2017
View reviewed changes
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.

👍 LGTM, will merge when green

@agrare agrare merged commit 3438f9b into ManageIQ:master Aug 1, 2017
@agrare agrare added this to the Sprint 66 Ending Aug 7, 2017 milestone Aug 1, 2017
@simaishi
Copy link
Contributor

simaishi commented Aug 2, 2017

Euwe backport (to manageiq repo) details:

$ git log -1
commit d2324d54de0b3803728583ed9ff29e6583628090
Author: Adam Grare <agrare@redhat.com>
Date:   Tue Aug 1 12:05:29 2017 -0400

    Merge pull request #77 from jameswnl/missingCtrl
    
    Dealing with disks which have no controller key
    (cherry picked from commit 3438f9b20d57c80e838874d0a8873b5dac4ededc)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1477727

simaishi pushed a commit that referenced this pull request Aug 9, 2017
@agrare @simaishi
Merge pull request #77 from jameswnl/missingCtrl
5b43c9c 
Dealing with disks which have no controller key
(cherry picked from commit 3438f9b)

https://bugzilla.redhat.com/show_bug.cgi?id=1479976
@simaishi
Copy link
Contributor

simaishi commented Aug 9, 2017

Fine backport details:

$ git log -1
commit 5b43c9c0f1ededc220616c0e4b9055b3486629ab
Author: Adam Grare <agrare@redhat.com>
Date:   Tue Aug 1 12:05:29 2017 -0400

    Merge pull request #77 from jameswnl/missingCtrl
    
    Dealing with disks which have no controller key
    (cherry picked from commit 3438f9b20d57c80e838874d0a8873b5dac4ededc)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1479976

agrare pushed a commit to agrare/manageiq-providers-vmware that referenced this pull request Apr 15, 2019
@gmcculloug
Merge pull request ManageIQ#77 from AndreyMenezes/XCS-113
3c3ff95 
Added LenovoXclarity Domain to EMS Events into Automate
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agrare agrare agrare approved these changes

Assignees

@agrare agrare

Labels
bug euwe/backported fine/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jameswnl @Fryguy @miq-bot @simaishi @agrare