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

Strategy: Add --silent flag to DEFAULT_CURL_ARGS #12693

Merged
merged 1 commit into from Jan 11, 2022

Conversation

samford
Copy link
Member

@samford samford commented Jan 10, 2022

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

This is a recreation of #12647, as I was encountering persistent CI failures that I didn't think were related to the committed changes (based on what I saw in testing). Digging into this further, it turned out that the failures were, in fact, due to the #curl_args commit where I used #uniq on the returned array. My intention was to avoid having options duplicated between args and extra_args (e.g., including --silent twice), simply for the sake of tidiness, but this approach causes problems because some options can be used more than once (e.g., --header).

Unfortunately, I modified the related branch, so I couldn't re-open the previous PR. That said, this PR is the same as the last one but I've dropped the #curl_args commit.


Currently, only Livecheck::Strategy::PAGE_HEADERS_CURL_ARGS uses the --silent option and PAGE_CONTENT_CURL_ARGS does not (though there's no intention behind this omission). However, the #page_content method should also use the --silent flag, to prevent progress bar text (#=#=#, etc.) from appearing in output.

This is an issue because the regex that's used to identify curl error messages in stderr (^curl:.+$/) will fail if leading progress bar text is present. This leads to an ambiguous "cURL failed without a detectable error" message instead of the actual error message(s) from curl.

This PR addresses the issue by adding --silent to Livecheck::Strategy::DEFAULT_CURL_ARGS, which both PAGE_HEADERS_CURL_ARGS and PAGE_CONTENT_CURL_ARGS inherit.

Currently, only `Livecheck::Strategy::PAGE_HEADERS_CURL_ARGS` uses
the `--silent` option and `PAGE_CONTENT_CURL_ARGS` does not (though
there's no intention behind this omission). However, the
`#page_content` method should also use the `--silent` flag, to
prevent progress bar text (`#=#=#`, etc.) from appearing in output.

This is an issue because the regex that's used to identify `curl`
error messages in `stderr` (`^curl:.+$/`) will fail if leading
progress bar text is present. This leads to an ambiguous "cURL
failed without a detectable error" message instead of the actual
error message(s) from `curl`.

This commit addresses the issue by adding `--silent` to
`Livecheck::Strategy::DEFAULT_CURL_ARGS`, which both
`PAGE_HEADERS_CURL_ARGS` and `PAGE_CONTENT_CURL_ARGS` inherit.
@samford samford added the critical Critical change which should be shipped as soon as possible. label Jan 10, 2022
@BrewTestBot
Copy link
Member

Review period skipped due to critical label.

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.

Thanks again @samford!

@MikeMcQuaid MikeMcQuaid merged commit 385892f into Homebrew:master Jan 11, 2022
@samford samford deleted the strategy-use-silent-flag branch January 11, 2022 14:47
@github-actions github-actions bot added the outdated PR was locked due to age label Feb 11, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
critical Critical change which should be shipped as soon as possible. outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants