Repository navigation
Conversation
The command's help and the README say every option can be given as an environment variable named after its flag, with flags taking precedence over the environment and the environment over defaults. Nothing read the environment, so $LOG_LEVEL and the rest were ignored. Before running, each flag not given on the command line is now set from its variable: the flag's name in capitals, with - as _. A value that does not parse is an error naming the variable. Options were also validated in NewCommand, before any flag was parsed, so only the defaults were ever checked. They are now validated after the command line and the environment are both applied. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HGACRqRjRF3TqH6B9vVijh Signed-off-by: Brian Kane <briankane1@gmail.com>
briankane
requested review from
FogDong,
Somefive,
anoop2811,
charlie0129 and
jguionnet
as code owners
October 15, 2026 20:24
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #70 +/- ##
==========================================
+ Coverage 45.39% 48.61% +3.21%
==========================================
Files 31 32 +1
Lines 1454 1475 +21
==========================================
+ Hits 660 717 +57
+ Misses 794 758 -36
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Review follow-ups. - --action-retry was documented, and had its constant and option field, but was never registered, so neither it nor $ACTION_RETRY could turn on retries. It is now a flag like the rest. - A validation error for a value from the environment says which variables were applied, so WORKERS=0 or LOG_LEVEL=bogus point at the variable to fix. A bad log level now names --log-level too. - The test of defaults clears every flag's variable first, so nothing the test process inherits can change the result. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HGACRqRjRF3TqH6B9vVijh Signed-off-by: Brian Kane <briankane1@gmail.com>
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
Each test applies every flag's variable, so an inherited LOG_LEVEL or WORKERS could fail one that sets something else. Every test now clears them all before setting its own; the tests pass with LOG_LEVEL=bogus, WORKERS=0 and MAX_RETRY=7 in the environment. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HGACRqRjRF3TqH6B9vVijh Signed-off-by: Brian Kane <briankane1@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of your changes
kubetrigger --helpand the README's configuration reference say every option can be given as an environment variable named after its flag ($LOG_LEVELfor--log-level), with the precedence command line, then environment, then default. Nothing read the environment, so these variables were ignored.Before running, each flag not given on the command line is now set from its variable: the flag's name in capitals, with
-as_. A value that does not parse is an error naming the variable ($MAX_RETRY: invalid argument "lots" ...).Options were also validated in
NewCommand, before any flag was parsed, so only the defaults were ever checked:--workers=0passed validation. They are now validated after the command line and the environment are both applied.I have:
make reviewableto ensure this PR is ready for review. (Ranmake lint,make checklicenseandgo mod tidy; no generated code is affected.)backport release-x.ylabels to auto-backport this PR if necessary. Not needed: kube-trigger has no release branches.How has this code been tested
pkg/cmd/env_test.go: a variable sets a flag not given; a flag given on the command line beats its variable; the default applies with neither; names map-to_for ints, bools and durations (PER_WORKER_QPS,LEADER_ELECT,LEADER_ELECTION_LEASE_DURATION); a bad value names its variable; and validation sees the final values (WORKERS=0is refused).go test ./pkg/... ./controllers/...passes; golangci-lint and staticcheck are clean.Special notes for your reviewer
Independent of the other open kube-trigger PRs. #68 adds
--enable-cel-expressionswith its README ENV entry markedTODO; whichever of the two merges second should set it toENABLE_CEL_EXPRESSIONS.Summary by cubic
Honors the environment variables
kubetriggerdocuments for its flags ($LOG_LEVELfor--log-level, and so on), and fixes flag validation so it no longer runs before flags are parsed.$FLAG_NAME(capitals,-as_), with precedence command line, then environment, then default; a value that fails to parse errors naming the variable.PreRunEafter the command line and environment are applied, so invalid final values likeWORKERS=0are rejected with an error naming the variable or flag, and--action-retry(documented but never registered) is now wired up alongside$ACTION_RETRY.LOG_LEVEL=boguscan't break them.Written for commit 67b744a. Summary will update on new commits.