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

Apache: Extend URL_MATCH_REGEX #12261

Merged

Conversation

samford
Copy link
Member

@samford samford commented Oct 19, 2021

  • 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?

apache-pulsar is the only formula in homebrew/core using an Apache URL that doesn't follow the prevailing /dyn/closer.lua?path=... format. The Apache strategy doesn't match the formula's current stable URL (https://www.apache.org/dyn/mirrors/mirrors.cgi?action=download&filename=pulsar/pulsar-2.8.0/apache-pulsar-2.8.0-src.tar.gz) and livecheck ends up checking the Git tags from head instead. We prefer to align the check with the stable URL whenever possible, so it's appropriate to extend the Apache strategy to address this.

It's been suggested in Homebrew/homebrew-core#87587 that we should simply update the URL to use the prevailing format (i.e., https://www.apache.org/dyn/closer.lua?path=pulsar/pulsar-2.8.0/apache-pulsar-2.8.0-src.tar.gz does work). However, the first-party download page uses the mirrors.cgi URL, so it's possible for the URL to switch back to the format that the Apache strategy doesn't support (e.g., if someone copies the URL from the page and uses it in a version bump).

We could potentially add an audit to enforce the closer.lua URL format for Apache URLs but I think there's value in extending the Apache strategy to support the mirrors.cgi URL format regardless.

@BrewTestBot
Copy link
Member

Review period will end on 2021-10-20 at 15:56:31 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Oct 19, 2021
@MikeMcQuaid
Copy link
Member

We could potentially add an audit to enforce the closer.lua URL format for Apache URLs but I think there's value in extending the Apache strategy to support the mirrors.cgi URL format regardless.

I'd prefer this personally but don't feel super strongly.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Approving as the actual change is very minimal.

@samford
Copy link
Member Author

samford commented Oct 19, 2021

We could potentially add an audit to enforce the closer.lua URL format for Apache URLs but I think there's value in extending the Apache strategy to support the mirrors.cgi URL format regardless.

I'd prefer this personally but don't feel super strongly.

We can add the audit in addition to extending the Apache strategy, so it makes sense to me. I'll go ahead and update the URL in the related homebrew/core PR and work on a separate brew PR for the URL audit.

@MikeMcQuaid
Copy link
Member

We can add the audit in addition to extending the Apache strategy, so it makes sense to me. I'll go ahead and update the URL in the related homebrew/core PR and work on a separate brew PR for the URL audit.

I think it's fine to leave as-is after seeing this change, actually 👍🏻

@BrewTestBot BrewTestBot added waiting for feedback Merging is blocked until sufficient time has passed for review and removed waiting for feedback Merging is blocked until sufficient time has passed for review labels Oct 19, 2021
Some Apache URLs use a `filename` query string parameter instead of
`path` and the `Apache` strategy isn't applied. The parameter value
is the same as `path` (i.e., a path to a file), so this updates the
strategy's `URL_MATCH_REGEX` to handle this URL format as well.
@samford samford force-pushed the livecheck/apache-extend-url-match-regex branch from ad07353 to e709917 Compare October 20, 2021 15:04
@samford
Copy link
Member Author

samford commented Oct 20, 2021

The latest push only includes a small tweak, to place path before filename in the regex group (i.e., (?:path|filename)). path is the parameter found in the overwhelming majority of our Apache URLs, so putting it at the front of the group could perform infinitesimally better over a large number of executions (e.g., checking a large tap like homebrew/core). It's not very meaningful but there's no harm in it, as the regex continues to match the same URLs.

@BrewTestBot
Copy link
Member

Review period ended.

@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Oct 20, 2021
@samford samford merged commit 83002b0 into Homebrew:master Oct 20, 2021
@samford samford deleted the livecheck/apache-extend-url-match-regex branch October 20, 2021 22:45
@github-actions github-actions bot added the outdated PR was locked due to age label Nov 20, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 20, 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

3 participants