Handles relative URLs in community_rest. #1800
Conversation
Signed-off-by: Jeffry Hesse <jeffryxtron@gmail.com>
|
|
||
| def relative_to_absolute_url(target) | ||
| if !URI.parse(target).absolute? | ||
| "#{api_uri.chomp("/api/v1")}#{target}" |
There was a problem hiding this comment.
this hardcoded "/api/v1" string smells brittle.
There was a problem hiding this comment.
Yeah, I didn't like that either. One of the biggest things here is I don't have access to source as far as I can tell, which is what I would actually prefer to use. I didn't want to go wild adding that in though.
There was a problem hiding this comment.
In reflecting on this class, I think quite a few methods could be moved out to downloader.rb, if the intent of this class is to just issue requests, etc...
If that is accomplished, then what I'm doing becomes significantly easier.
There was a problem hiding this comment.
I am confused about chomping "/api/v1" here. Here's the spec I cobbled together with my expectations (which may be wrong!):
describe "#relative_to_absolute_url" do
it "passes an already-absolute URL through unmodified" do
url = "https://somewhere.over.the.rain.bow/api/v1/cookbooks/wizardry"
expect(subject.relative_to_absolute_url(url)).to eq(url)
end
it "returns an absolute URL to a Supermarket endpoint given a relative URL path" do
url_path = "cookbooks/relativity"
expect(subject.relative_to_absolute_url(url_path)).to eq("https://supermarket.chef.io/api/v1/cookbooks/relativity")
end
end
... and the failure for the second case ...
1) Berkshelf::CommunityREST#relative_to_absolute_url returns an absolute URL to a Supermarket endpoint given a relative URL path
Failure/Error: expect(subject.relative_to_absolute_url(url_path)).to eq("https://supermarket.chef.io/api/v1/cookbooks/relativity")
expected: "https://supermarket.chef.io/api/v1/cookbooks/relativity"
got: "https://supermarket.chef.iocookbooks/relativity"
(compared using ==)
# ./spec/unit/berkshelf/community_rest_spec.rb:225:in `block (3 levels) in <top (required)>'
I am totally game to fix this however it needs to support relative targets. We'll need to include some specs and some words about expectations and why things are the way they are.
This lil ditty does something similar to what I did in #1799 , but inside of community_rest.rb, where it is not super stoked on relative URLs, as well.