feat(cli): -r/--refresh flag overrides list auto-refresh interval - #3
feat(cli): -r/--refresh flag overrides list auto-refresh interval#3posquit0 wants to merge 2 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new CLI flag -r / --refresh to override the auto-refresh intervals for lists and logs in the ota tool, along with corresponding resolution helper functions and unit tests. The reviewer feedback highlights an inconsistency where 0 is used as the default CLI flag value (meaning "use config") but represents "disable auto-refresh" in the configuration file. To resolve this, the reviewer suggests changing the default CLI flag value to -1 (to mean "use config") so that 0 can be used via the CLI to explicitly disable auto-refresh. Additionally, the reviewer recommends enforcing a minimum refresh interval safeguard of 500ms to prevent UI thread starvation and API rate-limiting, and updating the resolution logic and unit tests to match these new semantics.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| var refreshSec float64 | ||
| fs.Float64Var(&refreshSec, "r", 0, "list auto-refresh interval in seconds (overrides config; 0 = use config)") | ||
| fs.Float64Var(&refreshSec, "refresh", 0, "list auto-refresh interval in seconds (overrides config; 0 = use config)") |
There was a problem hiding this comment.
In the configuration file, a value of 0 is used to disable auto-refresh. However, in the CLI flags, 0 is used as the default value to mean "use config". This inconsistency makes it impossible for a user to explicitly disable auto-refresh via the CLI if it is enabled in the configuration file.
To resolve this inconsistency and allow disabling auto-refresh via the CLI, we can change the default value of the CLI flags to -1 (representing "use config") and let 0 explicitly mean "disable auto-refresh", matching the configuration file's semantics.
| var refreshSec float64 | |
| fs.Float64Var(&refreshSec, "r", 0, "list auto-refresh interval in seconds (overrides config; 0 = use config)") | |
| fs.Float64Var(&refreshSec, "refresh", 0, "list auto-refresh interval in seconds (overrides config; 0 = use config)") | |
| var refreshSec float64 | |
| fs.Float64Var(&refreshSec, "r", -1, "list auto-refresh interval in seconds (overrides config; 0 = disable, -1 = use config)") | |
| fs.Float64Var(&refreshSec, "refresh", -1, "list auto-refresh interval in seconds (overrides config; 0 = disable, -1 = use config)") |
| func resolveDefaultRefreshInterval(cfg config.RefreshConfig, overrideSec float64) time.Duration { | ||
| if overrideSec > 0 { | ||
| return time.Duration(overrideSec * float64(time.Second)) | ||
| } | ||
| return time.Duration(cfg.DefaultSeconds) * time.Second | ||
| } |
There was a problem hiding this comment.
To support the new -1 default value (representing "use config") and allow 0 to explicitly disable auto-refresh, we should update resolveDefaultRefreshInterval to check overrideSec >= 0.
Additionally, we should enforce a reasonable minimum refresh interval (e.g., 500 * time.Millisecond) for any positive override to prevent UI thread starvation and Okta API rate-limiting if a user accidentally passes an extremely small value (e.g., -r 0.001).
func resolveDefaultRefreshInterval(cfg config.RefreshConfig, overrideSec float64) time.Duration {
if overrideSec >= 0 {
if overrideSec == 0 {
return 0
}
d := time.Duration(overrideSec * float64(time.Second))
const minInterval = 500 * time.Millisecond
if d < minInterval {
return minInterval
}
return d
}
return time.Duration(cfg.DefaultSeconds) * time.Second
}| func resolveLogsRefreshInterval(cfg config.RefreshConfig, overrideSec float64) time.Duration { | ||
| if overrideSec > 0 { | ||
| return time.Duration(overrideSec * float64(time.Second)) | ||
| } | ||
| return time.Duration(cfg.LogsSeconds) * time.Second | ||
| } |
There was a problem hiding this comment.
To support the new -1 default value (representing "use config") and allow 0 to explicitly disable auto-refresh, we should update resolveLogsRefreshInterval to check overrideSec >= 0.
Additionally, we should enforce a reasonable minimum refresh interval (e.g., 500 * time.Millisecond) for any positive override to prevent UI thread starvation and Okta API rate-limiting if a user accidentally passes an extremely small value (e.g., -r 0.001).
func resolveLogsRefreshInterval(cfg config.RefreshConfig, overrideSec float64) time.Duration {
if overrideSec >= 0 {
if overrideSec == 0 {
return 0
}
d := time.Duration(overrideSec * float64(time.Second))
const minInterval = 500 * time.Millisecond
if d < minInterval {
return minInterval
}
return d
}
return time.Duration(cfg.LogsSeconds) * time.Second
}| func Test_ResolveRefreshInterval_CLIOverridesConfig(t *testing.T) { | ||
| cfg := config.RefreshConfig{LogsSeconds: 10, DefaultSeconds: 10} | ||
|
|
||
| t.Run("cli override wins on both surfaces", func(t *testing.T) { | ||
| assert.Equal(t, 2*time.Second, resolveDefaultRefreshInterval(cfg, 2)) | ||
| assert.Equal(t, 2*time.Second, resolveLogsRefreshInterval(cfg, 2)) | ||
| }) | ||
|
|
||
| t.Run("fractional seconds honored", func(t *testing.T) { | ||
| assert.Equal(t, 500*time.Millisecond, resolveDefaultRefreshInterval(cfg, 0.5)) | ||
| assert.Equal(t, 500*time.Millisecond, resolveLogsRefreshInterval(cfg, 0.5)) | ||
| }) | ||
|
|
||
| t.Run("zero override falls through to config", func(t *testing.T) { | ||
| assert.Equal(t, 10*time.Second, resolveDefaultRefreshInterval(cfg, 0)) | ||
| assert.Equal(t, 10*time.Second, resolveLogsRefreshInterval(cfg, 0)) | ||
| }) | ||
|
|
||
| t.Run("negative override treated as no override", func(t *testing.T) { | ||
| assert.Equal(t, 10*time.Second, resolveDefaultRefreshInterval(cfg, -3)) | ||
| assert.Equal(t, 10*time.Second, resolveLogsRefreshInterval(cfg, -3)) | ||
| }) | ||
|
|
||
| t.Run("cli override wins even when config disables auto-refresh", func(t *testing.T) { | ||
| disabled := config.RefreshConfig{LogsSeconds: 0, DefaultSeconds: 0} | ||
| assert.Equal(t, 2*time.Second, resolveDefaultRefreshInterval(disabled, 2)) | ||
| assert.Equal(t, 2*time.Second, resolveLogsRefreshInterval(disabled, 2)) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Update the unit tests to align with the new CLI override semantics: -1 represents "use config", 0 explicitly disables auto-refresh, and positive values below 500ms are capped to the minimum interval safeguard.
func Test_ResolveRefreshInterval_CLIOverridesConfig(t *testing.T) {
cfg := config.RefreshConfig{LogsSeconds: 10, DefaultSeconds: 10}
t.Run("cli override wins on both surfaces", func(t *testing.T) {
assert.Equal(t, 2*time.Second, resolveDefaultRefreshInterval(cfg, 2))
assert.Equal(t, 2*time.Second, resolveLogsRefreshInterval(cfg, 2))
})
t.Run("fractional seconds honored", func(t *testing.T) {
assert.Equal(t, 500*time.Millisecond, resolveDefaultRefreshInterval(cfg, 0.5))
assert.Equal(t, 500*time.Millisecond, resolveLogsRefreshInterval(cfg, 0.5))
})
t.Run("extremely small fractional seconds capped to minimum", func(t *testing.T) {
assert.Equal(t, 500*time.Millisecond, resolveDefaultRefreshInterval(cfg, 0.1))
assert.Equal(t, 500*time.Millisecond, resolveLogsRefreshInterval(cfg, 0.1))
})
t.Run("zero override disables auto-refresh", func(t *testing.T) {
assert.Equal(t, 0*time.Second, resolveDefaultRefreshInterval(cfg, 0))
assert.Equal(t, 0*time.Second, resolveLogsRefreshInterval(cfg, 0))
})
t.Run("negative override (default -1) falls through to config", func(t *testing.T) {
assert.Equal(t, 10*time.Second, resolveDefaultRefreshInterval(cfg, -1))
assert.Equal(t, 10*time.Second, resolveLogsRefreshInterval(cfg, -1))
})
t.Run("cli override wins even when config disables auto-refresh", func(t *testing.T) {
disabled := config.RefreshConfig{LogsSeconds: 0, DefaultSeconds: 0}
assert.Equal(t, 2*time.Second, resolveDefaultRefreshInterval(disabled, 2))
assert.Equal(t, 2*time.Second, resolveLogsRefreshInterval(disabled, 2))
})
}
k9s uses -r 2 to set the poll cadence. ota's DefaultRefreshInterval was config-only. Add a CLI flag so operators can override without touching config. - cmd/ota/main.go: register -r and --refresh (shared float64 target) - cmd/ota/wire.go: resolveDefaultRefreshInterval + resolveLogsRefreshInterval apply the CLI override to both surfaces; zero falls through to cfg.Refresh - cmd/ota/wire_test.go: resolver semantics + CLI parsing contract
Reworks the -r/--refresh semantics from PR review feedback:
- Flag default is now -1 ("use config") so that 0 can mean explicit disable,
matching the config file semantics.
- resolveDefaultRefreshInterval / resolveLogsRefreshInterval accept any
non-negative override (0 disables, positive clamps up to a 500ms floor)
and only fall through to config on negative values.
- Sub-500ms polling is clamped up to 500ms to avoid self-inflicted 429s.
- Tests updated to cover negative-uses-config, zero-disables, small-positive
clamped-to-floor, and positive-above-floor pass-through.
f95e869 to
7355c52
Compare
1 participant
Summary
-r 2for poll cadence. ota'sDefaultRefreshIntervalwas config-only-r/--refresh <sec>flag that overrides bothDefaultRefreshIntervalandLogsRefreshIntervalFiles
cmd/ota/main.go— register flag under both-rand--refresh(shared float64 target)cmd/ota/wire.go—resolveDefaultRefreshInterval+resolveLogsRefreshIntervalcmd/ota/wire_test.go— resolver semantics + CLI parsing contractTest plan
go test ./... -race -count=1greenota -r 5, watch Users list poll every 5s