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

cmd/update-report: nudge people to tweak settings. #14592

Merged
merged 1 commit into from
Jul 5, 2023
Merged

cmd/update-report: nudge people to tweak settings. #14592

merged 1 commit into from
Jul 5, 2023

Conversation

MikeMcQuaid
Copy link
Member

Nudge people who have set update-related settings to change them.

Inspired by @Bo98 pointing out we may not "get back" people who disabled the API.

It probably makes sense to hold off until 4.0.1 or later rather than 4.0.0 for this just so they get the most stable experience.

I will also add this to the 4.0.0 release notes.

@BrewTestBot
Copy link
Member

Review period will end on 2023-02-13 at 19:28:49 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Feb 10, 2023
@apainintheneck
Copy link
Contributor

This makes sense to me and I agree that it should probably wait until we're sure that installing from the API is stable.

@Bo98
Copy link
Member

Bo98 commented Feb 12, 2023

It probably makes sense to hold off until 4.0.1 or later rather than 4.0.0 for this just so they get the most stable experience.

This seems reasonable, I agree. A week of no bug reports could be the target before merging this.

@MikeMcQuaid
Copy link
Member Author

@RandomDSdevel want to explain the drive-by 👎🏻?

@MikeMcQuaid
Copy link
Member Author

A week of no bug reports could be the target before merging this.

👍🏻 good call.

@RandomDSdevel
Copy link
Contributor

RandomDSdevel commented Feb 13, 2023

@RandomDSdevel want to explain the drive-by 👎🏻?

     So that comes from a couple inter-related points that I originally refrained from bringing up immediately, at least partly so as not to derail the thread. I'll break these up a bit by category and/or environment variable:

  • 'HOMEBREW_NO_INSTALL_FROM_API:'
    • …Maybe users who've run a developer command could be exempt from getting nudged to unset at least 'HOMEBREW_NO_INSTALL_FROM_API?'
    • (And/or one of the commands on the 'doesn't work with that set' exemption list, though there isn't any state noting that down saved anywhere at the moment.)
    • (Maybe this'd actually be acceptable? I'm not sure.)
    • Some users, myself included, are likely to have set this to bound Homebrew's Internet/network use/access only to commands which absolutely need it, like 'brew update' and 'brew fetch' run by itself or as part of a 'brew install,' 'upgrade,' or 'reinstall' invocation, regardless of connection speed.
  • Some users, myself included, keep 'HOMEBREW_NO_AUTO_UPDATE' set because they prefer 'brew update' and commands that would run it automatically to stay distinct operations. (This partly derives from and was prompted by originally being around as a user when auto-update was first getting implemented.)
  • Not sure about 'HOMEBERW_AUTO_UPDATE_SECS,' as I don't use it, but I'd presume that users who do would've set it to reduce network/Internet use/accesses even if they don't mind Homebrew's auto-update behavior.

I just made a note to myself to set the Git configuration setting also introduced by this PR before running 'brew update' the next time I touch the machine I was using Homebrew on — it's been a fair bit since I have, as the external drive I was booting it from died and I've still yet to restore it from Time Machine — and moved on, though. 🤷‍♂️ We'll see if any other users comment on or complain about this after it gets implemented, I guess?

@Bo98
Copy link
Member

Bo98 commented Feb 13, 2023

Something just to clarify here, in case it wasn't known: this message would only display once and never again (like the analytics message).

@RandomDSdevel
Copy link
Contributor

     I know; I was just noting that the Git configuration setting's there to stop it from appearing at all if you don't want to see it and/or it's just noise for you. It's so trivial it barely registers as an inconvenvince or blip on the radar, though. (Continues carrying on with things.)
     (In other words, some users are as eagle-eyed as ever and like documenting things, though said documentation was more in my local notes. Move along, move along; nothing to see here.)

@MikeMcQuaid
Copy link
Member Author

refrained from bringing up immediately, at least partly so as not to derail the thread

For future reference: I consider a 👎🏻 with no comments to be pretty rude. If GitHub gave me the ability to disable: I would. In future: please drop a comment instead of a drive-by negative reaction, thanks ❤️

  • Maybe users who've run a developer command could be exempt from getting nudged to unset at least 'HOMEBREW_NO_INSTALL_FROM_API?'

I still think there's value for those users. I've been beta-testing the API for months and have found Homebrew to be generally faster and less annoying when doing so.

Something just to clarify here, in case it wasn't known: this message would only display once and never again (like the analytics message).

Yup, this. All of the other points I feel are pretty easily dismissed with "this is a message you will see once per machine and can then choose to ignore it".

@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Feb 13, 2023
@BrewTestBot
Copy link
Member

Review period ended.

@MikeMcQuaid MikeMcQuaid changed the title cmd/update-report: nudge people to tweak settings. cmd/update-report: nudge people to tweak API settings. Feb 16, 2023
@MikeMcQuaid MikeMcQuaid changed the title cmd/update-report: nudge people to tweak API settings. cmd/update-report: nudge people to tweak settings. Feb 16, 2023
@MikeMcQuaid
Copy link
Member Author

I've also updated this PR to nudge people on analytics settings so we can hopefully get some more people back to using analytics once we're off Google.

Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Happy with this. I suggest the same timeline of a week with no issues before shipping this.

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale No recent activity label Apr 19, 2023
@MikeMcQuaid MikeMcQuaid added in progress Maintainers are working on this and removed do not merge labels Apr 19, 2023
@MikeMcQuaid MikeMcQuaid removed the stale No recent activity label Apr 27, 2023
@MikeMcQuaid
Copy link
Member Author

Note: I am still planning on landing this, I was just waiting for:

  • us to migrate analytics entirely to InfluxDB (completed today ✅)
  • more of the install from api issues to get solved (working on that 🔜)

@Bo98
Copy link
Member

Bo98 commented Jun 16, 2023

  • more of the install from api issues to get solved (working on that 🔜)

Been working on a couple of those, but had no time this week. Will get a couple PRs up this weekend.

@MikeMcQuaid
Copy link
Member Author

@Bo98 Thanks! If you don't get the chance to open working PRs, feel free to push broken draft PRs. Similarly, if you're even before that but close to a solution: note in the issue or assign yourself so I don't duplicate your work. 🙇🏻

Library/Homebrew/cmd/update-report.rb Outdated Show resolved Hide resolved
Library/Homebrew/cmd/update-report.rb Outdated Show resolved Hide resolved
Library/Homebrew/cmd/update-report.rb Outdated Show resolved Hide resolved
Library/Homebrew/cmd/update-report.rb Outdated Show resolved Hide resolved
Nudge people who have set update or analytics related settings to change
them and to run `brew untap` for taps they no longer need.
@MikeMcQuaid MikeMcQuaid marked this pull request as ready for review July 5, 2023 16:19
@MikeMcQuaid
Copy link
Member Author

Last install with api issue closed so: merging.

@MikeMcQuaid MikeMcQuaid merged commit 66fc022 into Homebrew:master Jul 5, 2023
24 checks passed
@MikeMcQuaid MikeMcQuaid deleted the env_update_prompts branch July 5, 2023 17:28
@bevanjkay
Copy link
Member

bevanjkay commented Jul 6, 2023

@MikeMcQuaid I haven't traced the cause yet, but I received a prompt today on my second machine to

==> You have set:
  HOMEBREW_API_AUTO_UPDATE_SECS
but we have dramatically sped up and fixed many bugs in the way we do Homebrew updates since.
Please consider unsetting these and tweaking the values based on the new behaviour.

But echo $HOMEBREW_API_AUTO_UPDATE_SECS is empty


Is there a way I can easily get brew update-report to run manually so that I can test?

@MikeMcQuaid
Copy link
Member Author

@bevanjkay should be fixed by #15637

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

Successfully merging this pull request may close these issues.