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

Switch to HTTPS for 360 Casks #16134

Merged
merged 1 commit into from
Dec 27, 2015
Merged

Conversation

tsparber
Copy link
Contributor

Every changed URL was:

  • reachable using a valid HTTPS connection and
  • 'curl head' returned a status code of 200.

I did not verify the whole download, as I'm currently on limited bandwidth. Some casks redirect from HTTPS to HTTP (e.q. sourceforge links) those are left with HTTP.

Should I add more verification?

The script used can be found at cask-switch-https. Does this script also fit somewhere into homebrew-cask? I would happily create a pull request.

Log testing all Casks with a http url/appcast/homepage: https://gist.github.com/tsparber/da281e1912fc0a1ced20

As usually there should be one PR per changed cask: Should I create 360 separate PRs?

@tsparber
Copy link
Contributor Author

Hmm travis fails:

The log length has exceeded the limit of 4 MB (this usually means that the test suite is raising the same exception over and over).

The job has been terminated

So creating 362 PR is the way to go?

@jawshooah
Copy link
Contributor

The tests failed before it ever got that far:

  1) Failure:
Hbc::CLI::Home#test_0001_opens the homepage for the specified Cask [/Users/travis/build/caskroom/homebrew-cask/test/cask/cli/home_test.rb:25]:
--- expected
+++ actual
@@ -1 +1 @@
-[["/usr/bin/open", "--", "http://www.alfredapp.com/"]]
+[["/usr/bin/open", "--", "https://www.alfredapp.com/"]]

  2) Failure:
Hbc::CLI::Home#test_0002_works for multiple Casks [/Users/travis/build/caskroom/homebrew-cask/test/cask/cli/home_test.rb:32]:
--- expected
+++ actual
@@ -1 +1 @@
-[["/usr/bin/open", "--", "http://www.alfredapp.com/"], ["/usr/bin/open", "--", "https://www.adium.im/"]]
+[["/usr/bin/open", "--", "https://www.alfredapp.com/"], ["/usr/bin/open", "--", "https://www.adium.im/"]]

@tsparber
Copy link
Contributor Author

Fixed the test, at least the tests seem to work know. Audit is still running.

@vitorgalvao
Copy link
Member

There are merge conflicts that need to be solved.

Does this script also fit somewhere into homebrew-cask?

developer/bin. You don’t need to submit it there, though, we can just link to it from maintaining.md, if you prefer, but it seems like a nice fit.

I have a few scripts that are regularly used by some maintainers (and even users) here, but I keep them in my own repo for a few reasons. Some of them aren’t homebrew-cask specific and can easily benefit other projects and the one that is specific to us (cask-repair) isn’t really targeted for development, and can be used by users. I also very much like to keep them organised and even have formulas for them all all my others “tiny-scripts”. I never gave it any thought, really, but perhaps (at least cask-repair) could make sense here.

Up to you if you want to submit it here, but it seems like you do, and it could be a nice addition.

Looking through the script, I was amazed at how much our style for bash scripting is alike (even down to when to use $var vs ${var}) though I see differences in other areas, but looking further into it it seems like you may have gotten a lot from my scripts directly.

No problem there, naturally, they are public domain, after all. But I wonder if it should duplicate so much functionality (like fetching casks), since it is non-essential functionality and it means the script will essentially have to “catch up” that part of the code to cask-repair.

Perhaps it should be more focused in checking the availability of https, and doing it for all casks. We can worry about that later, though. It also seems like something that would be nice to add to cask-repair, so perhaps it would make sense to have two scripts:

  • cask-repair for individual casks (with the added functionality of checking if https is available.
  • cask-switch-https that just checks for https urls (but always does it for all casks).

@jawshooah
Copy link
Contributor

Looking through the script, I was amazed at how much our style for bash scripting is alike

That's no coincidence 😉

Every changed URL was reachable using valid HTTPS and returned a
StatusCode of 200.
@tsparber tsparber changed the title Switch to HTTPS for 362 Casks Switch to HTTPS for 360 Casks Dec 26, 2015
@tsparber
Copy link
Contributor Author

There are merge conflicts that need to be solved.

Rebased.

Up to you if you want to submit it here, but it seems like you do, and it could be a nice addition.

Great, I will then create a PR.

Looking through the script, I was amazed at how much our style for bash scripting is alike (even down to when to use $var vs ${var}) though I see differences in other areas, but looking further into it it seems like you may have gotten a lot from my scripts directly.
No problem there, naturally, they are public domain, after all.

Yes, this script is intentionally based on your cask-repair to keep things similar, thanks for the 'template' 😃

Perhaps it should be more focused in checking the availability of https, and doing it for all casks.

I cleaned the script to focus on https, by default it performs the verify now and runs on all casks (with the option to run an individual cask if provided).

@vitorgalvao
Copy link
Member

Checked manually and everything seems to be fine. Nice going on also fixing references to alfred’s url on the documentation and tests.

For future reference, github diffing limits. Seems like two PRs would’ve been enough, here, for us to at least be able to see all the files in the preview.

vitorgalvao added a commit that referenced this pull request Dec 27, 2015
@vitorgalvao vitorgalvao merged commit 17c34aa into Homebrew:master Dec 27, 2015
@tsparber tsparber deleted the switch-https branch December 28, 2015 02:57
@vitorgalvao vitorgalvao mentioned this pull request Jan 7, 2016
8 tasks
@Homebrew Homebrew locked and limited conversation to collaborators May 8, 2018
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.

3 participants