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

Fix Exception due to missing #merged_uri parameters in FileDepot parent class #18131

Merged
merged 3 commits into from Oct 25, 2018

Conversation

Projects
None yet
4 participants
@jerryk55
Member

jerryk55 commented Oct 25, 2018

The #merged_uri method in FileDepot was added by a previous PR but it was
missing its parameters, causing an exception when a derived class other than FileDepotSwift
is invoked.

Fix the parent FileDepot class, and add tests in two derived classes -
FileDepotNfs (which previously had no tests) and FileDepotFtp.
It is already tested in FileDepotSwift.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1643106

@yrudman @roliveri please review. @carbonin FYI.

Links [Optional]

Steps for Testing/QA

Schedule a DB backup on an NFS File Depot. It should not cause an exception.

Fix #merged_uri parameters in FileDepot parent class
The #merged_uri method in FileDepot was added by a previous PR but it was
missing its parameters, causing an exception when a derived class other than FileDepotSwift
is invoked.

Fix the parent FileDepot class, and add tests in two derived classes -
FileDepotNfs (which previously had no tests) and FileDepotFtp.
It is already tested in FileDepotSwift.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1643106

@jerryk55 jerryk55 changed the title from Fix #merged_uri parameters in FileDepot parent class to Fix Exception due to missing #merged_uri parameters in FileDepot parent class Oct 25, 2018

@@ -33,7 +33,7 @@ def upload_file(file)
@file = file
end
def merged_uri
def merged_uri(uri, _api_port)

This comment has been minimized.

@carbonin

carbonin Oct 25, 2018

Member

Doesn't this change the behavior? Before it was returning the attribute, now it's returning the parameter. Is that what we want?

This comment has been minimized.

@jerryk55

jerryk55 Oct 25, 2018

Member

It should be the same, but I realize there may be cases where its not - so we can ignore the parameter (via _uri). Changing and pushing it back out. Thanks @carbonin

Ignore uri parameter and return attribute instead
We really want to return the attribute, so ignore the same-named parameter
as previously intended.
@carbonin

This comment has been minimized.

Member

carbonin commented Oct 25, 2018

@jerryk55 can you create another spec that shows that the passed in uri is not used? So I guess pass both parameters as nil or something?

Add #merged_uri tests with nil and different uris
Instead of simply passing the same uri as returned from the attribute of the File Depot,
add tests passing in nil uri parameters which will show that the uri returned is the attribute.
Also add another test that passes in a different uri than is set on the attribute.
@jerryk55

This comment has been minimized.

Member

jerryk55 commented Oct 25, 2018

@carbonin ok I changed the tests that had been there, passing in nil uri parameters that are ignored, which return the uri attribute set on the FileDepots. Also added another test that passes in a Swift URI to the NFS file depot, showing that the attribute set on the depot is the one passed back and not the parameter.

@miq-bot

This comment has been minimized.

Member

miq-bot commented Oct 25, 2018

Checked commits jerryk55/manageiq@5bca881~...a99f71f with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 👍

@carbonin carbonin self-assigned this Oct 25, 2018

@carbonin carbonin merged commit 2357413 into ManageIQ:master Oct 25, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.004%) to 73.003%
Details

@carbonin carbonin added this to the Sprint 98 Ending Nov 5, 2018 milestone Oct 25, 2018

simaishi added a commit that referenced this pull request Oct 26, 2018

Merge pull request #18131 from jerryk55/fix_merged_uri
Fix Exception due to missing #merged_uri parameters in FileDepot parent class

(cherry picked from commit 2357413)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1643106
@simaishi

This comment has been minimized.

Contributor

simaishi commented Oct 26, 2018

Hammer backport details:

$ git log -1
commit 5d09d80b3a12c7a093d68389f79eed1a8940b314
Author: Nick Carboni <ncarboni@redhat.com>
Date:   Thu Oct 25 18:04:03 2018 -0400

    Merge pull request #18131 from jerryk55/fix_merged_uri
    
    Fix Exception due to missing #merged_uri parameters in FileDepot parent class
    
    (cherry picked from commit 23574133410fac78ef8c7aa95e06decbac3517ee)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1643106
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment