Skip to content

Add TOOLHIVE_SKIP_UPDATE_CHECK env var to disable update checks#5264

Merged
lujunsan merged 3 commits into
mainfrom
skip-update-check-env-var
May 12, 2026
Merged

Add TOOLHIVE_SKIP_UPDATE_CHECK env var to disable update checks#5264
lujunsan merged 3 commits into
mainfrom
skip-update-check-env-var

Conversation

@lujunsan
Copy link
Copy Markdown
Contributor

Summary

  • pkg/updates.ShouldSkipUpdateChecks() previously detected
    only CI environments via a fixed list of well-known CI env
    vars. There was no clean way for a user or downstream
    distribution to disable update checks in a non-CI runtime
    such as the operator.
  • Adds TOOLHIVE_SKIP_UPDATE_CHECK as an explicit opt-out
    env var. When set to "true" (case-insensitive),
    ShouldSkipUpdateChecks() returns true, which skips update
    checks from the CLI (cmd/thv/app/commands.go), the API
    server middleware (pkg/api/server.go), and the operator
    telemetry service (pkg/operator/telemetry/telemetry.go).
  • Update-derived usage metrics are also skipped, since
    usagemetrics.shouldEnableMetrics gates on the same
    function. This coupling is intentional.

Type of change

  • New feature (non-breaking change which adds
    functionality)

Test plan

  • Verified go build ./pkg/updates/... and go vet ./pkg/updates/... pass.
  • Manually traced all three call sites (CLI, API
    middleware, operator telemetry) to confirm the new opt-out
    gates each path through the existing
    ShouldSkipUpdateChecks() entry point.

Does this introduce a user-facing change?

Yes. Operators of the binary or the Kubernetes operator can
set TOOLHIVE_SKIP_UPDATE_CHECK=true to disable the update
check and the usage-metrics collection it gates.

ShouldSkipUpdateChecks() previously detected only CI environments via a
fixed list of well-known CI env vars. There was no clean way for a user
or downstream distribution to disable update checks in a non-CI runtime
such as the operator.

Add TOOLHIVE_SKIP_UPDATE_CHECK as an explicit opt-out env var. When set
to "true" (case-insensitive), ShouldSkipUpdateChecks() returns true,
which skips update checks from the CLI, the API server middleware, and
the operator telemetry service. Update-derived usage metrics are also
skipped, since usagemetrics.shouldEnableMetrics gates on the same
function -- this is the intended coupling.
@lujunsan lujunsan requested a review from JAORMX as a code owner May 12, 2026 11:55
@github-actions github-actions Bot added size/XS Extra small PR: < 100 lines changed and removed size/XS Extra small PR: < 100 lines changed labels May 12, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 12, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.12%. Comparing base (9d9604a) to head (d389da6).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5264      +/-   ##
==========================================
- Coverage   68.12%   68.12%   -0.01%     
==========================================
  Files         617      617              
  Lines       63066    63068       +2     
==========================================
- Hits        42964    42962       -2     
- Misses      16896    16902       +6     
+ Partials     3206     3204       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Member

@rdimitrov rdimitrov left a comment

Choose a reason for hiding this comment

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

Approach looks good — small change, plugs into the right place. A few things I'd like to see before this goes in:

  1. Add a unit test for the new env var path. pkg/updates/client_test.go is already the home for ShouldSkipUpdateChecks tests. Cover true / TRUE / unset (with CI vars cleared via t.Setenv).

  2. Heads-up: this also turns off usage metrics, because usagemetrics.ShouldEnableMetrics calls ShouldSkipUpdateChecks (pkg/usagemetrics/collector.go:28). You mention this in the PR body, but the env var name doesn't hint at it. Two options:

    • Leave the coupling, but note it on the constant's doc comment AND somewhere user-facing (operator Helm values / README) so people don't get surprised.
    • Decouple: only update checks read this var; usage metrics keeps its own TOOLHIVE_USAGE_METRICS_ENABLED knob. Bigger change — fine to do separately.
  3. Bool parsing style: usagemetrics uses exact == "false"; this PR uses strings.EqualFold(..., "true"). Not a blocker, but strconv.ParseBool would handle both styles (1, t, TRUE, etc.) and feels more idiomatic.

  4. Discoverability for operator users: since the motivation is the K8s operator, can we surface this in deploy/charts/operator/values.yaml (and the README that lists env vars)? Otherwise operators won't know it exists.

  5. Nit: consider moving the new constant next to ciEnvVars so all the "things that decide skip" live together.

lujunsan added 2 commits May 12, 2026 14:50
- Add TestShouldSkipUpdateChecks_SkipEnvVar covering true / TRUE / unset
  / false / unrecognized values. Clears CI env vars via t.Setenv so the
  opt-out path is exercised regardless of how the test binary is run.
- Move EnvVarSkipUpdateCheck next to ciEnvVars so all "things that
  decide skip" live together. Expand the doc comment to spell out the
  usage-metrics coupling.
- Document the env var on the operator chart's `operator.env` field so
  it is discoverable from values.yaml and the auto-generated chart
  README. Run helm-docs.
@lujunsan lujunsan force-pushed the skip-update-check-env-var branch from 808d003 to d389da6 Compare May 12, 2026 12:52
@github-actions github-actions Bot added size/XS Extra small PR: < 100 lines changed and removed size/XS Extra small PR: < 100 lines changed labels May 12, 2026
@lujunsan lujunsan requested a review from rdimitrov May 12, 2026 13:18
Copy link
Copy Markdown
Member

@rdimitrov rdimitrov left a comment

Choose a reason for hiding this comment

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

Thanks — this addresses everything from the previous round:

  1. TestShouldSkipUpdateChecks_SkipEnvVar covers the cases I asked for, and clearing the CI vars inside the loop makes the test deterministic regardless of where it runs.
  2. ✅ Const moved next to ciEnvVars, and the doc comment now spells out the usage-metrics coupling — that's the discoverability hook I was after.
  3. ✅ Helm chart values.yaml + auto-generated README both call it out.

Two tiny things, both non-blocking:

  • The PR body's Test plan still only mentions go build / go vet — worth ticking the unit-test box now that the test exists.
  • I'm intentionally not pushing on strconv.ParseBool vs EqualFold — current behavior matches what you documented ("true" case-insensitive), so leaving it is fine.

LGTM from my side once the test plan checkbox is updated.

@lujunsan lujunsan merged commit 89f99c0 into main May 12, 2026
47 of 48 checks passed
@lujunsan lujunsan deleted the skip-update-check-env-var branch May 12, 2026 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XS Extra small PR: < 100 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants