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

Try getting page content with both headers #11517

Merged
merged 1 commit into from
Jun 15, 2021

Conversation

n-thumann
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 style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Some casks are depending on user_agent: :fake, like hopper-debugger-server, keycue or hook (after merging Homebrew/homebrew-cask#106972). When running livecheck on them, the fake UA will not affect the request for fetching the version and therefore fail.

# brew livecheck hopper-debugger-server keycue
curl: (22) The requested URL returned error: 403 
Error: hopper-debugger-server: Unable to get versions
curl: (22) The requested URL returned error: 403 Forbidden
Error: keycue: Unable to get versions

This PR changes page_content(url) livecheck strategy to try both user agents (:default and :browser). The changes are analogue to b9741dd. It basically wraps the existing code inside an each loop.
With this PR applied the request will try both headers and the livecheck will succeed:

# brew livecheck hopper-debugger-server keycue
hopper-debugger-server : 2.7 ==> 2.7
keycue : 9.9 ==> 9.9

@MikeMcQuaid MikeMcQuaid requested review from samford, reitermarkus and a team June 10, 2021 11:29
@vitorgalvao
Copy link
Member

This isn’t a huge issue overall, so no strong feelings on the implementation. But it’s worth considering instead a user_agent parameter, like in url. It would be consistent with what we have, allow for specific user agents to be given when necessary, and be explicit (avoiding surprises).

I mention this because while a “wrong” user agent typically leads to failure to get a page, on occasion it leads to getting the wrong page (typically the Windows version).

@samford
Copy link
Member

samford commented Jun 10, 2021

I will be removing the multiple user-agent logic from Strategy#page_headers after I've introduced a PR for my [finished] work on allowing configuration options in livecheck blocks. This will support configuring the user-agent (among other necessary use cases) and I would very much prefer that we wait and take that approach.

This will happen as soon as I've finished my open Homebrew/brew PRs but I'm trying not too spin too many plates at once. However, it may be best if I simply create a draft PR and I can mark it as ready for review when my other PRs are finished. This would make the changes visible and give me something to link to, as there have been a number of related requests lately.


In general, I was against the idea of making a follow-up request with a different user agent in #page_headers when the HeaderMatch strategy was introduced. I approved this logic at the time because the HeaderMatch strategy is only manually applied, it wouldn't affect a large number of casks/formulae, and I believe I was still working on allowing configuration options at the time. Even then, my opinion was that this is something that should be handled manually by specifying the user-agent in a livecheck block (not by automatically making multiple requests).

#page_content is used by almost 75% of strategies, some of which are applied automatically, so the bar for approval in this PR is higher. This may automatically fix checks for a small number of casks but it could come with unwanted side effects in other formulae/casks. A request can fail for reasons other than the user-agent and shouldn't be retried in those scenarios.


That said, I will at least test this out (with some additional debugging information added to the JSON output) to collect concrete evidence on how this PR behaves. I'll have to do a few runs across homebrew/core and homebrew/cask, so it will take a few hours at a minimum.

Copy link
Member

@samford samford left a comment

Choose a reason for hiding this comment

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

Testing this for homebrew/core, this change didn't affect any checks that use #page_content. Testing this for homebrew/cask, this change only affects the casks you already mentioned (hook, hopper-debugger-server, keycue) and camo-studio (which also requires the :browser user-agent).

Like the related #page_headers logic, this change does lead to two requests where we only need one but livecheck doesn't offer an alternative yet, it only affects a few casks, and it will be removed in the near future.

Since the undesirable side effects of this are minimal at the moment, this can technically be merged. If we do, I'll have to walk back these changes as part of my work to allow for configuration options in livecheck blocks but that shouldn't be too much trouble.

I would have preferred to leave these four checks broken until I introduce configuration options in the near future but I'm amenable to merging this since it's your first Homebrew/brew PR.

@MikeMcQuaid MikeMcQuaid merged commit 06124a4 into Homebrew:master Jun 15, 2021
@MikeMcQuaid
Copy link
Member

Merging given @samford's comment.

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

@github-actions github-actions bot added the outdated PR was locked due to age label Jul 16, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants