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

brew: Add ScpDownloadStrategy #3938

Merged
merged 1 commit into from Apr 7, 2018

Conversation

Projects
None yet
2 participants
@ryangreenberg
Copy link
Contributor

ryangreenberg commented Mar 16, 2018

Checklist

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Description

This PR implements the change proposed in #3891.

I would like to make it easy to install bottles that are stored in a secure location to further enable internal/corporate use of homebrew. I propose doing this by introducing a file download strategy that uses scp.

The new ScpDownloadStrategy allows users to download source code (and with #3841, bottles) using scp. Here's what it looks like in practice:

$ head ./etl.rb 
class Etl < Formula
  # Note URL and :using => ScpDownloadStrategy
  url "scp://example.slack.com/homebrew/etl-0.04.19.tar.gz", :using => ScpDownloadStrategy
  sha256 "ba944c1a07fd321488f9d034467931b8ba9e48454abef502a633ff4835380c1c"

$ ./bin/brew fetch ./etl.rb
==> Downloading scp://example.slack.com/homebrew/etl-0.04.19.tar.gz
etl-0.04.19.tar.gz                    100%  363KB 701.9KB/s   00:00
Downloaded to: /Users/greenberg/Library/Caches/Homebrew/etl-0.04.19.tar.gz
SHA256: ba944c1a07fd321488f9d034467931b8ba9e48454abef502a633ff4835380c1c

As a convenient accident, since scp is run via system, it prompts the user for two-factor authentication or other credentials if required.

A couple implementation notes:

  • I reused CurlDownloadStrategyError when the URL pattern doesn't match. That's not really accurate. I can introduce a new name, simply use ArgumentError, or leave as-is.
  • I did not add scp to DownloadStrategyDetector.

@ryangreenberg ryangreenberg force-pushed the ryangreenberg:scp_download_strategy branch from be1fd67 to 0239862 Mar 19, 2018

@MikeMcQuaid
Copy link
Member

MikeMcQuaid left a comment

Thanks for the PR! First round of comments here.

end

def parse_url_pattern
url_pattern = %r{scp://([^@]+@)?([^@:/]+)(:[0-9]+)?/(\S+)}

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 20, 2018

Member

This regex is a bit gnarly. Can it e simplified at all? e.g. at least [0-9] can be \d

This comment has been minimized.

@ryangreenberg

ryangreenberg Mar 20, 2018

Contributor

I'll change [0-9] to \d.

As far as simplification, I'm not sure how much I can do to change it and still extract the desired parts. Two options:

  1. Use whitespace mode and document the regex with comments
  2. Put the regex in a constant and test it specifically so people changing it can at least see what behavior they need to maintain (this is already done indirectly via the tests for fetch but I'd be happy to move them to make it easier to understand/change)

def parse_url_pattern
url_pattern = %r{scp://([^@]+@)?([^@:/]+)(:[0-9]+)?/(\S+)}
unless @url =~ url_pattern

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 20, 2018

Member

Use if and !~

private

def scp_source
path_prefix = @path.start_with?("~") ? "" : "/"

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 20, 2018

Member

path_prefix = "/" if @path.start_with?("~")

This comment has been minimized.

@ryangreenberg

ryangreenberg Mar 20, 2018

Contributor

Nifty—never thought of doing it that way.

This comment has been minimized.

@ryangreenberg

ryangreenberg Mar 20, 2018

Contributor

Not sure if this changes your guidance, but I realized this would be path_prefix = "/" unless @path.start_with("~")


def scp_source
path_prefix = @path.start_with?("~") ? "" : "/"
port_arg = @port ? "-P #{@port[1..-1]} " : ""

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 20, 2018

Member

port_arg = "-P #{@port[1..-1]} " if @port

invalid_urls.each do |invalid_url|
context "with invalid URL #{invalid_url}" do
it "raises CurlDownloadStrategyError" do
expect { described_class.new(name, resource_for(invalid_url)) }.to raise_error(CurlDownloadStrategyError)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 20, 2018

Member

Split this (and the other really long lines) only multiple lines.

@ryangreenberg ryangreenberg force-pushed the ryangreenberg:scp_download_strategy branch from 0239862 to b360496 Mar 20, 2018

begin
safe_system "scp", scp_source, temporary_path.to_s
rescue ErrorDuringExecution
raise CurlDownloadStrategyError, @url

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 21, 2018

Member

As you've mentioned: this should be a different exception.

end

def parse_url_pattern
url_pattern = %r{scp://([^@]+@)?([^@:/]+)(:\d+)?/(\S+)}

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 21, 2018

Member

Can you supply an example test formula as a comment? I'll obviously have to edit it to match something I have scp access to but it would be helpful.

This comment has been minimized.

@ryangreenberg

ryangreenberg Apr 6, 2018

Contributor

Done!

@ryangreenberg ryangreenberg force-pushed the ryangreenberg:scp_download_strategy branch from de87473 to 6f0b7d4 Mar 22, 2018

@ryangreenberg ryangreenberg force-pushed the ryangreenberg:scp_download_strategy branch from 6f0b7d4 to d5b0730 Apr 6, 2018

@ryangreenberg

This comment has been minimized.

Copy link
Contributor

ryangreenberg commented Apr 6, 2018

OK, I'm back post-merge-conflicts-resolution! Thanks for your help on this PR, @MikeMcQuaid. Let me know if there's anything else you'd like to see or think is missing.

On a side note, if you think now (or later) that having this extra download strategy is a burden, I think you could probably remove it and ask people who want to use it to require it in their own formula.

@ryangreenberg ryangreenberg force-pushed the ryangreenberg:scp_download_strategy branch from d5b0730 to fe8939d Apr 6, 2018

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Apr 7, 2018

Thanks so much for your first contribution! Without people like you submitting PRs we couldn't run this project. You rock, @ryangreenberg!

@MikeMcQuaid MikeMcQuaid merged commit d0261d6 into Homebrew:master Apr 7, 2018

3 checks passed

codecov/patch 88.23% of diff hit (target 70.11%)
Details
codecov/project 70.16% (+0.04%) compared to 94c0d83
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ryangreenberg ryangreenberg deleted the ryangreenberg:scp_download_strategy branch Apr 16, 2018

@lock lock bot added the outdated label May 16, 2018

@lock lock bot locked as resolved and limited conversation to collaborators May 16, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.