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

Add audit check for URL schema #3318

Merged
merged 5 commits into from
Dec 5, 2017
Merged

Add audit check for URL schema #3318

merged 5 commits into from
Dec 5, 2017

Conversation

Git-Jiro
Copy link
Contributor

  • 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 tests with your changes locally?

This implements: Homebrew/homebrew-cask#25403

"--output", "/dev/null",
url_to_access,
user_agent: :fake
)
Copy link
Member

Choose a reason for hiding this comment

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

This will download the whole file to /dev/null, won't it? It should use --head.

@MikeMcQuaid
Copy link
Member

It would be good to share more code here with audit's equivalent.

@Git-Jiro
Copy link
Contributor Author

@MikeMcQuaid Ok, I will have a look.

@Git-Jiro
Copy link
Contributor Author

@MikeMcQuaid is this the kind of code reuse you envisioned?

@MikeMcQuaid
Copy link
Member

@Git-Jiro Yes, something like that. You may want to extract the FormulaAuditor.check_http_content into utils/curl instead as importing a Homebrew cmd/audit inside Cask seems a little weird. Wait for @reitermarkus's thoughts first, though.

@stale
Copy link

stale bot commented Nov 16, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale No recent activity label Nov 16, 2017
@Git-Jiro
Copy link
Contributor Author

@MikeMcQuaid Do you have an example for me where I can look at a similar refactoring? I would like to see how other people handled similar situations in the code base.

@stale stale bot removed the stale No recent activity label Nov 16, 2017
@MikeMcQuaid
Copy link
Member

@Git-Jiro I don't have a good example I can think of but I can answer more specific questions and provide review feedback.

@Git-Jiro
Copy link
Contributor Author

Git-Jiro commented Nov 20, 2017

Ok, sounds like a plan!

@ilovezfs ilovezfs added the cask Homebrew Cask label Nov 26, 2017
@Git-Jiro
Copy link
Contributor Author

Git-Jiro commented Dec 3, 2017

@MikeMcQuaid What do you think about my first version of this refactoring?

@MikeMcQuaid
Copy link
Member

@Git-Jiro Looks great, thanks for that! I'm happy to 🚢 this when @reitermarkus is.

@@ -275,6 +277,17 @@ def core_formula_url
"#{core_tap.default_remote}/blob/master/Formula/#{cask.token}.rb"
end

def check_https_availability
check_url_for_https_availability(cask.url) unless cask.url.to_s.empty?
Copy link
Member

Choose a reason for hiding this comment

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

This should probably also use cask.url.user_agent.

@@ -275,6 +277,18 @@ def core_formula_url
"#{core_tap.default_remote}/blob/master/Formula/#{cask.token}.rb"
end

def check_https_availability
user_agents = cask.url.user_agent.to_s.empty? ? [:default] : [cask.url.user_agent]
Copy link
Member

Choose a reason for hiding this comment

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

#user_agent already defaults to :default by itself.

@reitermarkus reitermarkus merged commit 497348a into Homebrew:master Dec 5, 2017
@reitermarkus
Copy link
Member

Thanks, @Git-Jiro, nice work here!

@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cask Homebrew Cask
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants