-
Notifications
You must be signed in to change notification settings - Fork 0
feat(cli): -r/--refresh flag overrides list auto-refresh interval #3
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,6 +29,12 @@ type WireInput struct { | |
| TokenEnv string | ||
| Debug bool | ||
| PollSec int | ||
| // RefreshSec, when >= 0, overrides both cfg.Refresh.DefaultSeconds | ||
| // and cfg.Refresh.LogsSeconds. Sourced from `-r`/`--refresh` | ||
| // (k9s-style). A negative value (the flag default -1) falls through | ||
| // to config; 0 explicitly disables auto-refresh; positive values | ||
| // below 500ms are clamped up to the 500ms floor. | ||
| RefreshSec float64 | ||
| } | ||
|
|
||
| // Wire is the single explicit dependency-assembly point. It loads config, | ||
|
|
@@ -126,10 +132,8 @@ func Wire(ctx context.Context, in WireInput) (app.Model, config.Config, error) { | |
| APITokensPort: oktaClient.APITokens(), | ||
| AdministratorsPort: oktaClient.Administrators(), | ||
| APIRecorder: apiRecorder, | ||
| LogsRefreshInterval: time.Duration(cfg.Refresh.LogsSeconds) * | ||
| time.Second, | ||
| DefaultRefreshInterval: time.Duration(cfg.Refresh.DefaultSeconds) * | ||
| time.Second, | ||
| LogsRefreshInterval: resolveLogsRefreshInterval(cfg.Refresh, in.RefreshSec), | ||
| DefaultRefreshInterval: resolveDefaultRefreshInterval(cfg.Refresh, in.RefreshSec), | ||
| // v0.2.2 #190 — kick the status.okta.com probe by default; | ||
| // override via cfg.OktaStatusEndpoint when self-hosted Okta | ||
| // orgs run a different statuspage (rare). | ||
|
|
@@ -181,6 +185,52 @@ func pickProfile(cfg config.Config, override string) (string, config.Profile, er | |
| return "", config.Profile{}, errors.New("ota: multiple profiles configured — pass --profile <name>") | ||
| } | ||
|
|
||
| // refreshFloor bounds how tight a CLI-provided interval can be. Sub-500ms | ||
| // polling risks self-inflicted 429s and burns operator terminals for no | ||
| // observable benefit; k9s and friends pick a similar floor. | ||
| const refreshFloor = 500 * time.Millisecond | ||
|
|
||
| // resolveRefreshOverride translates a CLI-provided override into a Duration | ||
| // according to the shared semantics (negative = no override; 0 = explicit | ||
| // disable; positive with a 500ms floor). Returns (duration, true) when the | ||
| // CLI value governs, or (0, false) when the caller should defer to config. | ||
| func resolveRefreshOverride(overrideSec float64) (time.Duration, bool) { | ||
| if overrideSec < 0 { | ||
| return 0, false | ||
| } | ||
| if overrideSec == 0 { | ||
| return 0, true | ||
| } | ||
| d := time.Duration(overrideSec * float64(time.Second)) | ||
| if d < refreshFloor { | ||
| d = refreshFloor | ||
| } | ||
| return d, true | ||
| } | ||
|
|
||
| // resolveDefaultRefreshInterval picks the cadence for non-Logs lists. A | ||
| // non-negative CLI override wins over config: 0 explicitly disables | ||
| // auto-refresh; positive values are clamped up to a 500ms floor. A | ||
| // negative override (the flag default -1) means "use config" and | ||
| // cfg.DefaultSeconds applies. | ||
| func resolveDefaultRefreshInterval(cfg config.RefreshConfig, overrideSec float64) time.Duration { | ||
| if d, ok := resolveRefreshOverride(overrideSec); ok { | ||
| return d | ||
| } | ||
| return time.Duration(cfg.DefaultSeconds) * time.Second | ||
| } | ||
|
|
||
| // resolveLogsRefreshInterval picks the cadence for the Logs tail. Same | ||
| // override semantics as resolveDefaultRefreshInterval. `-r` overrides | ||
| // both surfaces until a distinct `--logs-refresh` (or config) knob | ||
| // exists. | ||
| func resolveLogsRefreshInterval(cfg config.RefreshConfig, overrideSec float64) time.Duration { | ||
| if d, ok := resolveRefreshOverride(overrideSec); ok { | ||
| return d | ||
| } | ||
| return time.Duration(cfg.LogsSeconds) * time.Second | ||
| } | ||
|
Comment on lines
+227
to
+232
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To support the new Additionally, we should enforce a reasonable minimum refresh interval (e.g., 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 defaultDebugLogPath() (string, error) { | ||
| cache := os.Getenv("XDG_CACHE_HOME") | ||
| if cache == "" { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,95 @@ | ||
| package main | ||
|
|
||
| import ( | ||
| "os" | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
|
|
||
| "github.com/tedilabs/ota/internal/config" | ||
| ) | ||
|
|
||
| // Test_ResolveRefreshInterval_CLIOverridesConfig pins QW-8's contract: | ||
| // a non-negative `-r`/`--refresh` overrides both | ||
| // Deps.DefaultRefreshInterval and Deps.LogsRefreshInterval, regardless | ||
| // of what cfg.Refresh holds. A negative value (the flag default -1) | ||
| // falls through to config; 0 explicitly disables auto-refresh; | ||
| // positive values under 500ms are clamped up to the 500ms floor. | ||
| 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 at or above floor", func(t *testing.T) { | ||
| assert.Equal(t, 500*time.Millisecond, resolveDefaultRefreshInterval(cfg, 0.5)) | ||
| assert.Equal(t, 500*time.Millisecond, resolveLogsRefreshInterval(cfg, 0.5)) | ||
| assert.Equal(t, 1500*time.Millisecond, resolveDefaultRefreshInterval(cfg, 1.5)) | ||
| assert.Equal(t, 1500*time.Millisecond, resolveLogsRefreshInterval(cfg, 1.5)) | ||
| }) | ||
|
|
||
| t.Run("small positive override clamped up to 500ms floor", func(t *testing.T) { | ||
| assert.Equal(t, 500*time.Millisecond, resolveDefaultRefreshInterval(cfg, 0.1)) | ||
| assert.Equal(t, 500*time.Millisecond, resolveLogsRefreshInterval(cfg, 0.1)) | ||
| assert.Equal(t, 500*time.Millisecond, resolveDefaultRefreshInterval(cfg, 0.001)) | ||
| assert.Equal(t, 500*time.Millisecond, resolveLogsRefreshInterval(cfg, 0.001)) | ||
| }) | ||
|
|
||
| t.Run("zero override explicitly disables auto-refresh", func(t *testing.T) { | ||
| assert.Equal(t, time.Duration(0), resolveDefaultRefreshInterval(cfg, 0)) | ||
| assert.Equal(t, time.Duration(0), resolveLogsRefreshInterval(cfg, 0)) | ||
| }) | ||
|
|
||
| t.Run("negative override 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)) | ||
| 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)) | ||
| }) | ||
|
|
||
| t.Run("zero override still disables even when config would enable", func(t *testing.T) { | ||
| // Explicit `-r 0` beats a non-zero config — user asked for silence. | ||
| assert.Equal(t, time.Duration(0), resolveDefaultRefreshInterval(cfg, 0)) | ||
| assert.Equal(t, time.Duration(0), resolveLogsRefreshInterval(cfg, 0)) | ||
| }) | ||
| } | ||
|
Comment on lines
+20
to
+65
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Update the unit tests to align with the new CLI override semantics: 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))
})
} |
||
|
|
||
| // Test_RefreshFlag_ParsesBothShortAndLong pins the CLI contract: `-r` and | ||
| // `--refresh` are accepted as equivalent. run() short-circuits on | ||
| // --version so this exercises the flag parser without touching tea. | ||
| func Test_RefreshFlag_ParsesBothShortAndLong(t *testing.T) { | ||
| cases := []struct { | ||
| name string | ||
| args []string | ||
| }{ | ||
| {"short form -r", []string{"-r", "2", "--version"}}, | ||
| {"long form --refresh", []string{"--refresh", "2.5", "--version"}}, | ||
| {"long form -refresh (Go stdlib syntax)", []string{"-refresh=3", "--version"}}, | ||
| } | ||
| for _, tc := range cases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| stdout, err := os.CreateTemp(t.TempDir(), "stdout") | ||
| require.NoError(t, err) | ||
| stderr, err := os.CreateTemp(t.TempDir(), "stderr") | ||
| require.NoError(t, err) | ||
| code := run(tc.args, stdout, stderr) | ||
| require.NoError(t, stdout.Close()) | ||
| require.NoError(t, stderr.Close()) | ||
| errBytes, _ := os.ReadFile(stderr.Name()) | ||
| assert.Equal(t, 0, code, "flag parse must succeed; stderr was: %s", errBytes) | ||
| assert.NotContains(t, string(errBytes), "flag provided but not defined", | ||
| "both -r and --refresh must be registered") | ||
| }) | ||
| } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To support the new
-1default value (representing "use config") and allow0to explicitly disable auto-refresh, we should updateresolveDefaultRefreshIntervalto checkoverrideSec >= 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).