Skip to content

Update shouldCheckForUpdate#2134

Merged
mislav merged 4 commits into
cli:trunkfrom
ShubhankarKG:updates
Oct 9, 2020
Merged

Update shouldCheckForUpdate#2134
mislav merged 4 commits into
cli:trunkfrom
ShubhankarKG:updates

Conversation

@ShubhankarKG

Copy link
Copy Markdown
Contributor

Fixes #743

  1. Checks if CI Environment by checking if CI, BUILD_MANAGER, RUN_ID is non blank.
  2. Checks if Update Env is globally closed through GH_NO_UPDATE_NOTIFIER is "disabled"

@mislav mislav left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you!

Could you also add documentation for GH_NO_UPDATE_NOTIFIER in help_topic.go among other environment variables?

Comment thread cmd/gh/main.go Outdated
func shouldCheckForUpdate() bool {
return updaterEnabled != "" && !isCompletionCommand() && utils.IsTerminal(os.Stderr)
isCIEnvironment := (os.Getenv("CI") != "") || (os.Getenv("BUILD_NUMBER") != "") || (os.Getenv("RUN_ID") != "")
isGlobalDisabled := os.Getenv("GH_NO_UPDATE_NOTIFIER") == "disabled"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could this just check for GH_NO_UPDATE_NOTIFIER being set to any non-blank string, like the above checks?

The presence of "NO" in this variable implies that if the variable is set (to any value), update notifier will be disabled.

ShubhankarKG and others added 2 commits October 8, 2020 21:22
2. Updated test to check if passed variable is not empty

@mislav mislav left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fantastic; thank you! ❤️

@ShubhankarKG ShubhankarKG reopened this Oct 9, 2020
@ShubhankarKG

Copy link
Copy Markdown
Contributor Author

Apologies for closing. Clicked on the button by mistake 😅

@mislav mislav merged commit a2aa07d into cli:trunk Oct 9, 2020
@bader1572

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add a option to disable update notifications

4 participants