Fail Cinder/Swift Ensures if Service not Present #17067
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Some major issues with the tests caused by this - I'm working on that. |
jerryk55
force-pushed
the
fix_swift_refresher_when_no_swift
branch
from
d5f79e5 to
dbe9bc1
Compare
bdunne
reviewed
Mar 20, 2018
| @@ -34,7 +34,7 @@ def ems_inv_to_hashes | |||
| end | |||
|
|
|||
| def object_store | |||
| return if @swift_service.blank? || @swift_service.name != :swift | |||
| return if @swift_service.nil? || @swift_service.blank? || @swift_service.name != :swift | |||
There was a problem hiding this comment.
Any reason for adding the extra nil check? nil is blank, so it shouldn't be needed.
There was a problem hiding this comment.
Brain fart. I think I thought about changing it, not adding it. I'll revert that line...
roliveri
approved these changes
Mar 20, 2018
Return false for cinder and swift ensure calls if the service is not present. Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1458959 Requires a fix to the Openstack provider as well although order of merges is irrelevant.
In order to allow spec tests to pass the cloud_manager_spec must requires an authentication object for the openstack provider. Also further checks for validity in the refresh parser.
Review comments pointed out the nil check was extraneous.
Typo in safe nav operator - "&" -after the "." instead of before caused an error. Not really needed on the first class so made it match with the swift refresh_parser.
jerryk55
force-pushed
the
fix_swift_refresher_when_no_swift
branch
from
ae4d22c to
d2bf3c8
Compare
|
@roliveri I found an error In my log because of a typo in a safe navigation operator. Fixed and ready to be merged when travis has finished. |
|
Checked commits jerryk55/manageiq@73f42d4~...d2bf3c8 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 app/models/manageiq/providers/storage_manager/cinder_manager/refresh_parser.rb
|
|
@jerryk55 ManageIQ/manageiq-providers-openstack#240 is |
|
@miq-bot add_label fine/yes |
simaishi
pushed a commit
that referenced
this pull request
Mar 26, 2018
…wift Fail Cinder/Swift Ensures if Service not Present (cherry picked from commit 9b54d98) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1560692
|
Gaprindashvili backport details: |
simaishi
pushed a commit
that referenced
this pull request
Apr 9, 2018
…wift Fail Cinder/Swift Ensures if Service not Present (cherry picked from commit 9b54d98) https://bugzilla.redhat.com/show_bug.cgi?id=1560693
|
Fine backport details: |
d-m-u
pushed a commit
to d-m-u/manageiq
that referenced
this pull request
Jun 6, 2018
…hen_no_swift Fail Cinder/Swift Ensures if Service not Present (cherry picked from commit 9b54d98) https://bugzilla.redhat.com/show_bug.cgi?id=1560693
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Return false for cinder and swift ensure calls if the
service is not present.
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1458959
Requires ManageIQ/manageiq-providers-openstack#240
as well although order of merges is irrelevant.
In addition https://bugzilla.redhat.com/show_bug.cgi?id=1538501 fixes a related issue via PR
#16922
@roliveri @hsong-rh please review. Thanks.
Links