Skip to content
This repository has been archived by the owner on Apr 14, 2021. It is now read-only.

Permit absolute paths in cache_path setting #5628

Merged
merged 2 commits into from May 2, 2017

Conversation

mal
Copy link
Contributor

@mal mal commented Apr 30, 2017

Removes check on leading / from cache_path and updates tests.

Fixes #5627

@@ -23,12 +23,4 @@
expect(bundled_app("vendor/cache-foo/rack-1.0.0.gem")).to exist
end
end

context "when given an absolute path" do
it "exits with non-zero status" do
Copy link
Member

Choose a reason for hiding this comment

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

can we change this to a test that it packages correctly?

path = self[:cache_path] || "vendor/cache"
raise InvalidOption, "Cache path must be relative to the bundle path" if path.start_with?("/")
path
self[:cache_path] || "vendor/cache"
Copy link
Member

Choose a reason for hiding this comment

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

The begin block is no longer necessary here

DEFAULT_VENDOR_CACHE_PATH = "vendor/cache"
...

def app_cache_path
  @app_cache_path ||= self[:cache_path] || DEFAULT_VENDOR_CACHE_PATH
end

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've held off extracting "vendor/cache" into a constant for now because it didn't appear to fit with the rest of the file. It makes sense to do but perhaps as part of a separate wider scoped refactor.

@mal mal force-pushed the permit-absolute-cache-path branch from 08ead2f to 8fce595 Compare May 1, 2017 15:25
@segiddins
Copy link
Member

👍🏻

@segiddins
Copy link
Member

@bundlerbot r+

@bundlerbot
Copy link
Collaborator

📌 Commit 8fce595 has been approved by segiddins

@bundlerbot
Copy link
Collaborator

⌛ Testing commit 8fce595 with merge af27726...

bundlerbot added a commit that referenced this pull request May 2, 2017
Permit absolute paths in cache_path setting

Removes check on leading `/` from `cache_path` and updates tests.

Fixes #5627
@bundlerbot
Copy link
Collaborator

☀️ Test successful - status-travis
Approved by: segiddins
Pushing af27726 to master...

@bundlerbot bundlerbot merged commit 8fce595 into rubygems:master May 2, 2017
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

4 participants