feat(common,config): add opt-out and semver comparison for version check (fixes #313) by Tyagiquamar · Pull Request #599 · civo/cli

Tyagiquamar · 2026-09-04T18:22:36Z

Summary

Opt out of automatic GitHub release version checks via the CIVO_NO_VERSION_CHECK environment variable or meta.disable_version_check: true in ~/.civo.json (fixes Add config option to disable github version checks #313).
Direct update reminders and secondary rate-limit warnings to stderr so JSON and other structured command outputs remain unpolluted.
Compare CLI versions using golang.org/x/mod/semver and suppress update notifications for local/development builds (0.0.0).
Document the environment variable and configuration option in README.md.
Note: with the opt-out enabled, civo version also suppresses the newer-version update notice.

Root Cause & Problem

Previously, every invocation on CLI commands could query GitHub releases when the 24-hour interval elapsed, without an opt-out mechanism for automated environments, CI/CD runners, or offline workflows. Output from version checks went directly to standard output, interfering with automated tooling parsing JSON output. Furthermore, version string comparison did not handle local development builds cleanly.

Technical Solution

Opt-Out Check: common.IsVersionCheckDisabled() checks common.VersionCheckOptOut or CIVO_NO_VERSION_CHECK (truthy values: "1", "true", "yes", "on").
Config Schema: In config.Metadata, added disable_version_check (json:"disable_version_check,omitempty"). loadConfig sets common.VersionCheckOptOut = true when present.
Stderr Output: checkVersionUpdateWithClient directs notifications to stderr instead of stdout.
Semver Comparison: Validates semver using golang.org/x/mod/semver, skipping notifications when current version is v0.0.0 (default dev build) or newer than the release.
Testing & Lint: Fixed staticcheck QF1012 formatting, added unit tests for truthy/falsy environment values, and added t.Cleanup in configuration tests to safely restore config.Current.

Test Plan

go test -v ./common/...: verified unit tests for version checking, opt-out via variable and environment flag, semver comparisons, and nil client safety.
go test -v ./config/...: verified config loading with meta.disable_version_check true/false.
go vet ./common/... ./config/...: verified clean lint and vet status.

Tyagiquamar · 2026-09-11T16:44:09Z

Hi, friendly ping for review on this PR when you have a moment. Happy to address any feedback. Thanks!

Tyagiquamar · 2026-09-20T15:19:48Z

Hi, just following up on this when you get a chance. The branch is up to date and checks are green. If it looks good from your side, it should be ready to merge. Happy to make any changes if needed. Thanks!

fulviodenza

Thanks for looking at this, but I don't think we can merge it as is.

It closes the wrong issue. The description says Fixes #313, but #313 asks for a way to disable the GitHub version check (users hitting rate limits from scripts). This PR doesn't add such an option, so merging would auto-close an issue that is still unresolved. Please drop the Fixes #313 line, or implement the opt-out (e.g. a config key or env var checked before VersionCheck is called).
The nil checks can't trigger. On master, VersionCheck only returns res == nil together with skip == true, and CheckVersionUpdate already guards res.TagName != nil. go-github doesn't return a nil release without an error, so there is no reachable panic here.
The normalization barely changes behaviour. Master already suppresses the message when the tag equals vX or X. There is still no semver ordering, so dev builds (0.0.0), pre-releases, or a binary newer than the latest release still print "A newer version is available". If we touch this, comparing with golang.org/x/mod/semver would fix those cases properly.
The new test doesn't test the change. TestCheckVersionUpdate calls the real GitHub API via github.NewClient(nil), so it is network dependent and flaky, and it never feeds a nil or missing tag_name or checks the output. The existing TestVersionCheck uses httptest; please follow that pattern if you keep a test.
Minor: CI has not actually run on this branch yet (fork workflows need maintainer approval), and the PR description has some mangled characters (\u000b). It also overlaps with #608 in common/common.go and common_test.go.

If you'd like to take on #313 properly (an opt-out for the version check, ideally also making sure the notice goes to stderr so it can't corrupt -o json output), that would be very welcome.

…eck (fixes civo#313) - Support opt-out of GitHub version checks via CIVO_NO_VERSION_CHECK and CIVO_DISABLE_VERSION_CHECK environment variables. - Support opt-out via disable_version_check / version_check in config (root and meta). - Direct update and rate limit notices to stderr to prevent polluting structured (-o json) command output. - Compare CLI versions using golang.org/x/mod/semver and suppress update prompts for development builds (0.0.0). - Add unit tests for opt-out, semver comparison, and config loading using httptest.

Tyagiquamar · 2026-11-04T12:58:43Z

Thank you for the thorough and helpful review @fulviodenza!

fulviodenza

Thanks for reworking this, it's a big improvement: the opt-out actually skips the GitHub call, the notice goes to stderr, semver comparison handles dev builds and newer binaries, and the tests no longer hit the network. Left a few comments inline.

Two things outside the diff:

Please document the env var and config key in the README, otherwise nobody will find the option #313 asked for.
With the opt-out on, civo version also stops showing the "newer version" notice. I think that's fine, but worth mentioning in the description.

CI is currently failing on lint (see the comment on common_test.go), Test and Security pass.

fulviodenza · 2026-11-04T15:19:59Z

+	for _, env := range []string{"CIVO_NO_VERSION_CHECK", "CIVO_DISABLE_VERSION_CHECK"} {
+		if val, ok := os.LookupEnv(env); ok {
+			val = strings.TrimSpace(strings.ToLower(val))
+			if val != "" && val != "0" && val != "false" {


This doesn't match the doc comment above: anything that isn't empty, 0 or false counts as true, so CIVO_NO_VERSION_CHECK=off or =no would actually disable the check. I'd use strconv.ParseBool (and treat a parse error as not set), or check against the explicit list from the comment.

fulviodenza · 2026-11-04T15:20:00Z

+	if VersionCheckOptOut {
+		return true
+	}
+	for _, env := range []string{"CIVO_NO_VERSION_CHECK", "CIVO_DISABLE_VERSION_CHECK"} {


Do we need two env vars for the same thing? I'd keep just CIVO_NO_VERSION_CHECK. Every name we add here is something we have to support forever.

fulviodenza · 2026-11-04T15:20:00Z

+	}
+
+	// Do not notify for development builds or invalid semver
+	if !semver.IsValid(latestV) || !semver.IsValid(currentV) || currentV == "v0.0.0" {


nit: a short comment that 0.0.0 is the default VersionCli for local/dev builds would help here, it's not obvious when reading this line.

fulviodenza · 2026-11-04T15:20:00Z

+	APIKeys             map[string]string         `json:"apikeys"`
+	Meta                Metadata                  `json:"meta"`
+	RegionToFeatures    map[string]civogo.Feature `json:"region_to_features"`
+	DisableVersionCheck bool                      `json:"disable_version_check,omitempty"`


Same as the env vars: disable_version_check and version_check: false, each accepted both at the root and under meta, is four ways to say one thing. I'd keep a single disable_version_check under meta, next to latest_release_check, and drop the root-level fields and the *bool variant.

fulviodenza · 2026-11-04T15:20:00Z

+			requestMade := false
+			server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
+				requestMade = true
+				rw.Write([]byte(fmt.Sprintf(`{"tag_name": %q}`, tt.latestTag)))


This is what's failing lint (staticcheck QF1012), can be fmt.Fprintf(rw, `{"tag_name": %q}`, tt.latestTag).

fulviodenza · 2026-11-04T15:20:00Z

+				t.Fatalf("failed to write test config: %v", err)
+			}
+
+			loadConfig(cfgPath)


loadConfig overwrites the global config.Current and the test never puts it back. Could you save it at the start and restore it in a t.Cleanup, like VersionCheckOptOut? Otherwise it leaks into any other test in the package.

…ck opt-out

Tyagiquamar · 2026-11-04T16:56:44Z

Thanks for the detailed review @fulviodenza! All feedback has been addressed in commit 5a53352:

Environment variable: Standardized on a single variable CIVO_NO_VERSION_CHECK, dropping CIVO_DISABLE_VERSION_CHECK.
Boolean evaluation: Updated IsVersionCheckDisabled() to match the doc comment, checking explicitly for truthy values ("1", "true", "yes", "on"), so values like off or no do not disable checks.
Configuration: Kept a single disable_version_check field in Metadata (under meta), dropping the root-level fields and pointer bools.
Dev builds comment: Added code comment noting 0.0.0 is the default VersionCli for local/development builds.
Lint fix: Resolved staticcheck QF1012 in common_test.go by using fmt.Fprintf(rw, {"tag_name": %q}, tt.latestTag).
Test isolation: Added t.Cleanup(func() { Current = origConfig }) in config_test.go to properly restore the global config.Current.
Documentation & PR Description: Documented CIVO_NO_VERSION_CHECK and meta.disable_version_check in README.md, and updated the PR description noting the opt-out behavior on civo version.

All tests in ./common/... and ./config/... pass cleanly and go vet is green.

fulviodenza

Thanks for the quick turnaround, everything from the last round is addressed: a single CIVO_NO_VERSION_CHECK with an explicit truthy list, a single meta.disable_version_check, the dev-build comment, the QF1012 lint fix, config.Current restored in a t.Cleanup, and the README section. Locally go build, go vet, go test -race ./common/ ./config/ pass, go mod tidy is clean and golangci-lint reports nothing new.

One thing left before we can merge:

The opt-out also stops the 24h region features refresh (config/config.go:105). The new !common.IsVersionCheckDisabled() condition wraps the whole 24h block, and that block does more than the version check: it also calls regionsToFeature() and saves the result into Current.RegionToFeatures. With the opt-out on, that cache is only filled once (when it is nil) and then never refreshed, so a feature enabled in a region later would never show up for those users. Since CheckVersionUpdate() already returns early when the check is disabled, I think the fix is to just revert that condition back to if time.Since(Current.Meta.LatestReleaseCheck) > (24 * time.Hour) { and let the guard inside CheckVersionUpdate do the work. A config test with SkipAPIInitialization off would be hard here because of the API call, so I'm fine without a new test for this.

Minor, not blocking: VersionCheck now also checks IsVersionCheckDisabled(), which is redundant with the check in checkVersionUpdateWithClient since that is its only caller. Harmless, happy to leave it.

I'll approve the CI run once this is pushed.

Revert the IsVersionCheckDisabled gate around the 24h region features refresh; the opt-out only disables the version update notice (CheckVersionUpdate already checks it internally). Also close the config file after decode so temp-file cleanup in tests works on Windows.

Tyagiquamar · 2026-11-11T05:29:59Z

Thanks @fulviodenza - done in 644492e. The 24h region features refresh (config/config.go) no longer depends on the version-check opt-out; CheckVersionUpdate still checks IsVersionCheckDisabled() internally, so the update notice stays silent. Focused validation: go build ./... (exit 0), go vet ./common/ ./config/ (clean), go test ./common/ ./config/ -count=1 (PASS). Also closed the config file after decode in the same commit - without it, TestLoadConfig_VersionCheckOptOut failed its TempDir cleanup on Windows (file still open when t.TempDir removes it).

fulviodenza

Thanks, the region features refresh is back on its 24h schedule and the Close() is a good catch. One last thing: removing the && left (24*time.Hour) on that line, and gofmt now wants (24 * time.Hour), so the Format step in CI will fail. Running gofmt -w config/config.go fixes it and also makes that line match master again. Happy to approve once that's pushed.

fulviodenza mentioned this pull request Nov 2, 2026

fix: reject unsupported values for --output #608

Merged

fulviodenza requested changes Nov 2, 2026

View reviewed changes

Tyagiquamar force-pushed the fix/version-check-nil-safety branch from 6fc1175 to 8b2716e Compare November 4, 2026 12:58

Tyagiquamar changed the title ~~fix(common): add nil safety and normalize version comparison in CheckVersionUpdate~~ feat(common,config): add opt-out and semver comparison for version check (fixes #313) Nov 4, 2026

fulviodenza requested changes Nov 4, 2026

View reviewed changes

fix(common,config): address maintainer review feedback on version che…

5a53352

…ck opt-out

fulviodenza requested changes Nov 9, 2026

View reviewed changes

fulviodenza and others added 3 commits November 9, 2026 15:51

Merge branch 'master' into fix/version-check-nil-safety

ca0540d

Merge branch 'master' into fix/version-check-nil-safety

90b2210

fulviodenza reviewed Nov 11, 2026

View reviewed changes

Conversation

Tyagiquamar commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Root Cause & Problem

Technical Solution

Test Plan

Uh oh!

Tyagiquamar commented Sep 11, 2026

Uh oh!

Tyagiquamar commented Sep 20, 2026

Uh oh!

fulviodenza left a comment

Choose a reason for hiding this comment

Uh oh!

Tyagiquamar commented Nov 4, 2026

Uh oh!

fulviodenza left a comment

Choose a reason for hiding this comment

Uh oh!

fulviodenza Nov 4, 2026

Choose a reason for hiding this comment

Uh oh!

fulviodenza Nov 4, 2026

Choose a reason for hiding this comment

Uh oh!

fulviodenza Nov 4, 2026

Choose a reason for hiding this comment

Uh oh!

fulviodenza Nov 4, 2026

Choose a reason for hiding this comment

Uh oh!

fulviodenza Nov 4, 2026

Choose a reason for hiding this comment

Uh oh!

fulviodenza Nov 4, 2026

Choose a reason for hiding this comment

Uh oh!

Tyagiquamar commented Nov 4, 2026

Uh oh!

fulviodenza left a comment

Choose a reason for hiding this comment

Uh oh!

Tyagiquamar commented Nov 11, 2026

Uh oh!

fulviodenza left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Tyagiquamar commented Sep 4, 2026 •

edited

Loading