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

bump-cask-pr: deprecate online flag #15983

Merged
merged 1 commit into from
Sep 11, 2023

Conversation

razvanazamfirei
Copy link
Member

  • 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 PR deprecates brew bump-cask-pr --online and makes online audits the default setting.

PRs created via bump-cask-pr skip full auditing. Occasionally, the bumped cask version does not match with the livecheck version leading to PRs that fail CI and clogging the PR queue until the livecheck version catches up.

Given that a --no-audit flag already exists, adding an --offline flag seems redundant. However, if there are any edge cases I'm missing, happy to discuss more.

Copy link
Contributor

@apainintheneck apainintheneck left a comment

Choose a reason for hiding this comment

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

In general, this makes sense to me. Note that we usually try and wait for next major/minor release before changing deprecation status though. It might actually be nice to have a waiting to deprecate label for stuff like this.

@@ -67,6 +67,7 @@ def bump_cask_pr_args
def bump_cask_pr
args = bump_cask_pr_args.parse

odeprecated "brew bump-cask-pr --online" if args.online?
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be nice if we could customize the deprecation message some more. Something like --online is now on by default; use --no-audit to disable it would be helpful here. Not blocking though. It looks like deprecated would need to be modified in that case though. Or maybe something like:

if args.online?
  deprecated "brew bump-cask-pr --online"
  puts "`--online` is now the default. Use `--no-audit` to avoid online checks."
end

Copy link
Member

Choose a reason for hiding this comment

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

Comment this out for now and we'll add it on the next release.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bit tough though since there are other changes in the same file that need to be made in the same file. Probably better to just wait and do it all at once, right?

Copy link
Member

Choose a reason for hiding this comment

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

@apainintheneck I don't think these changes to a developer command need to wait until the next major/minor release. The odeprecated is more debatable and I'm fine either way. If we do want to add it: it'd be nicer to comment it out now otherwise it'll likely be bumped to odisabled on the next major/minor release which isn't what we want.

Copy link
Member Author

Choose a reason for hiding this comment

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

@MikeMcQuaid I was not sure if we meant commenting out all the changes, or just odeprecated, so let me know if I need to revert any portion.

Copy link
Member

Choose a reason for hiding this comment

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

@razvanazamfirei Sorry for confusion: only the odeprecated line needs commented out.

@MikeMcQuaid
Copy link
Member

Thanks again @razvanazamfirei!

@MikeMcQuaid MikeMcQuaid merged commit 86acd12 into Homebrew:master Sep 11, 2023
24 checks passed
@razvanazamfirei razvanazamfirei deleted the deprecate-online-flag branch September 11, 2023 13:35
@github-actions github-actions bot added the outdated PR was locked due to age label Oct 12, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 12, 2023
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.

3 participants