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

Allow relative urls in location_path for downloader #1799

Merged

Conversation

DarthHater
Copy link
Contributor

Hi there!

I'm working on a chef plugin for Nexus Repository, and I ran into some fun!

When using berkshelf to install, it initially hits /universe to get the chef dependency graph. This graph includes a bunch of absolute URLs, which makes for fun times when proxying a repository. I wrote a utility that sets all those URLs to relative, which would in theory work with a tool, and it did for knife (I don't think it uses universe), but berkshelf was not so happy. The reason we prefer relative URLs for Nexus Repo is because once we get into building a group type functionality (grouping multiple repositories under one accessible URL), it makes life a lot easier.

What I did in this PR was check to see if the location_path was set to relative, and if it is, I take the source.uri_string and append the two together, as the source.uri_string SHOULD be the location for Nexus Repo, etc... if your berksfile is configured correctly.

I am probably opening a can of worms with this PR, so feel free to let me know if this is a poorly thought out change.

A few tests appear to be failing but I believe they are unrelated to this change.

@robbkidd
Copy link

@DarthHater Thanks for this and for cookbook support in Nexus!

Imma play with these specs a little.


it "that are absolute and uses the given absolute URI" do
allow(remote_cookbook).to receive(:location_path) { "http://some.bucket.somewhere.example.com:8081/repository/chef-proxy/api/v1" }
expect(CommunityREST).to receive(:new).with("http://some.bucket.somewhere.example.com:8081/repository/chef-proxy/api/v1", { ssl: {} }) { rest }

Choose a reason for hiding this comment

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

I know I wrote this bit, but after looking at Downloader.try_download and CommunityREST.download, the some.bucket.somewhere.example.com I added here doesn't make sense. 🤦‍♂️

Downloader.try_download under test here is still poking at a particular Supermarket API endpoint. It's not until CommunityREST.download goes to retrieve the actual archive that a URL might be something other than the Supermarket endpoint (e.g. an S3 bucket for the actual cookbook artifact).

So, uh, some.bucket.somewhere.example.com can go back to being localhost to make sense in context. 😬

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In: 80977c3

From original PR:

Hi there!

I'm working on a chef plugin for Nexus Repository, and I ran into some fun!

When using berkshelf to install, it initially hits /universe to get the chef dependency graph. This graph includes a bunch of absolute URLs, which makes for fun times when proxying a repository. I wrote a utility that sets all those URLs to relative, which would in theory work with a tool, and it did for knife (I don't think it uses universe), but berkshelf was not so happy. The reason we prefer relative URLs for Nexus Repo is because once we get into building a group type functionality (grouping multiple repositories under one accessible URL), it makes life a lot easier.

What I did in this PR was check to see if the location_path was set to relative, and if it is, I take the source.uri_string and append the two together, as the source.uri_string SHOULD be the location for Nexus Repo, etc... if your berksfile is configured correctly.

I am probably opening a can of worms with this PR, so feel free to let me know if this is a poorly thought out change.

A few tests appear to be failing but I believe they are unrelated to this change.

Signed-off-by: Jeffry Hesse <jeffryxtron@gmail.com>
@robbkidd
Copy link

I endorse these force pushes.

force push

Copy link

@robbkidd robbkidd left a comment

Choose a reason for hiding this comment

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

it's all relative

@lamont-granquist
Copy link
Contributor

image

@lamont-granquist lamont-granquist merged commit 5aff05c into berkshelf:master Nov 27, 2018
@lock
Copy link

lock bot commented Jan 26, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants