feat: make default event severity filter configurable#4646
feat: make default event severity filter configurable#4646zyzzmohit wants to merge 1 commit intokubernetes-sigs:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: zyzzmohit The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
Adds a new backend-configurable default for the Cluster Overview “Only warnings” events filter, while keeping per-user overrides via localStorage.
Changes:
- Backend: introduces
filters-warnings-onlyconfig/flag and exposes it via/config. - Frontend: stores
filtersWarningsOnlyin Redux and uses it as the default for the events filter toggle (when no user preference exists). - Helm: adds
config.events.warningsOnlyand passes it as a backend CLI arg.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/redux/configSlice.ts | Adds filtersWarningsOnly to Redux config state and attempts to hydrate it from /config. |
| frontend/src/components/cluster/Overview.tsx | Uses Redux filtersWarningsOnly as the default for the “Only warnings” toggle. |
| frontend/package-lock.json | Large lockfile churn (adds "peer": true entries broadly). |
| charts/headlamp/values.yaml | Adds Helm value config.events.warningsOnly (default true). |
| charts/headlamp/templates/deployment.yaml | Passes -filters-warnings-only arg from Helm values. |
| backend/pkg/headlampconfig/headlampConfig.go | Adds FiltersWarningsOnly (and PrometheusEndpoint) to runtime config struct. |
| backend/pkg/config/config.go | Adds FiltersWarningsOnly to parsed config and a new CLI flag. |
| backend/cmd/stateless.go | Includes FiltersWarningsOnly in the /parseKubeConfig response payload. |
| backend/cmd/headlamp.go | Extends /config response payload with filtersWarningsOnly (and prometheusEndpoint). |
Files not reviewed (1)
- frontend/package-lock.json: Language not supported
Comments suppressed due to low confidence (1)
frontend/src/components/cluster/Overview.tsx:121
- The warning-only switch state is initialized from
filtersWarningsOnlyonly once (in theuseStateinitializer). Because the app doesn’t block rendering while/configis fetched,filtersWarningsOnlycan update after this component mounts, but the switch state will not follow—so the admin-configured default may never apply for first-time users (no localStorage key). Consider syncingisWarningEventSwitchCheckedwhenfiltersWarningsOnlychanges and no localStorage value exists, or derive the initial state lazily and update via an effect until the user explicitly toggles.
const filtersWarningsOnly = useTypedSelector(state => state.config.filtersWarningsOnly);
const EVENT_WARNING_SWITCH_DEFAULT = filtersWarningsOnly;
const { t } = useTranslation(['translation', 'glossary']);
const location = useLocation();
const queryParams = new URLSearchParams(location.search);
const eventsFilter = queryParams.get('eventsFilter');
const filterFunc = useFilterFunc<Event>(['.jsonData.involvedObject.kind']);
const [isWarningEventSwitchChecked, setIsWarningEventSwitchChecked] = React.useState(
Boolean(
JSON.parse(
localStorage.getItem(EVENT_WARNING_SWITCH_FILTER_STORAGE_KEY) ||
EVENT_WARNING_SWITCH_DEFAULT.toString()
)
)
);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| setConfig( | ||
| state, | ||
| action: PayloadAction<{ clusters: ConfigState['clusters']; filtersWarningsOnly: boolean }> | ||
| ) { | ||
| state.clusters = action.payload.clusters; | ||
| if (action.payload.filtersWarningsOnly !== undefined) { | ||
| state.filtersWarningsOnly = action.payload.filtersWarningsOnly; | ||
| } | ||
| }, |
There was a problem hiding this comment.
setConfig now requires filtersWarningsOnly, but multiple call sites dispatch setConfig({ clusters }) (e.g. auth chooser) and unit tests also call it without this field. This will break TypeScript builds/tests, and the !== undefined guard is currently inconsistent with the payload type. Make filtersWarningsOnly optional in the action payload (or accept a broader partial config payload) and keep the default in initialState when it’s omitted; update the reducer tests accordingly.
| // FiltersWarningsOnly is the default state of the events filter (true = only warnings, false = all events). | ||
| FiltersWarningsOnly bool `koanf:"filters-warnings-only"` |
There was a problem hiding this comment.
FiltersWarningsOnly is added to the parsed Config and exposed as a CLI flag, but it also needs to be mapped into the runtime headlampconfig.HeadlampCFG (the struct used by /config via c.FiltersWarningsOnly). Currently the HeadlampCFG builder (in backend/cmd/server.go) does not set this field, so the value from flags/env will never reach the API response. Please wire conf.FiltersWarningsOnly into the HeadlampCFG construction so the frontend can receive the configured default.
| {{- with .Values.config.tlsKeyPath }} | ||
| - "-tls-key-path={{ . }}" | ||
| {{- end }} | ||
| - "-filters-warnings-only={{ .Values.config.events.warningsOnly }}" |
There was a problem hiding this comment.
This argument directly renders .Values.config.events.warningsOnly without a default/with guard. If a user overrides config without the new events subtree (or sets it to {}), Helm can render <no value> here, producing an invalid -filters-warnings-only=<no value> flag and failing startup. Use default true (or conditionally emit the arg) to make the template robust.
| type clientConfig struct { | ||
| Clusters []Cluster `json:"clusters"` | ||
| IsDynamicClusterEnabled bool `json:"isDynamicClusterEnabled"` | ||
| PrometheusEndpoint string `json:"prometheusEndpoint"` |
There was a problem hiding this comment.
PrometheusEndpoint is now part of the /config response, but it doesn’t appear to be populated anywhere in the backend config parsing (no flags/env mapping found) and the frontend doesn’t reference prometheusEndpoint. If this field isn’t intentionally being introduced as part of this feature, consider removing it from clientConfig to avoid expanding the API surface with an always-empty value; otherwise ensure it’s properly configured and consumed.
| PrometheusEndpoint string `json:"prometheusEndpoint"` |
frontend/package-lock.json
Outdated
| "resolved": "https://registry.npmjs.org/@babel/core/-/core-7.28.3.tgz", | ||
| "integrity": "sha512-yDBHV9kQNcr2/sUr9jghVyz9C3Y5G2zUM2H2lo+9mKv4sFgbA8s8Z9t8D1jiTkGoO/NoIfKMyKWr4s6CN23ZwQ==", | ||
| "license": "MIT", | ||
| "peer": true, |
There was a problem hiding this comment.
Large, broad lockfile churn here adds "peer": true across many packages without any corresponding dependency changes in this PR. If the lockfile update isn’t required for the feature, it would be better to revert it (or regenerate it using the repo’s pinned Node/NPM versions) to keep the PR focused and reduce merge conflicts.
| "peer": true, |
illume
left a comment
There was a problem hiding this comment.
Thanks for this.
Please consider the review comments?
Also, please check the git commit messages match the format we use. See the git commit message guidelines in the contributing guide.
|
Thanks for the reviews! I've addressed the TypeScript compilation errors by making |
4c9d97f to
0c046a0
Compare
Add a configuration option to set the default state of the events warning filter. Configurable via Helm chart value 'config.events.warningsOnly' or backend flag '-filters-warnings-only'. The frontend reads this value as the default if the user has not set a preference. Signed-off-by: zyzzmohit <mohitray949@gmail.com>
0c046a0 to
3030251
Compare
|
@illume Thanks for the guidance! I've addressed the feedback and cleaned up the branch:
Ready for another look :) |
Summary
This PR introduces a new configuration option to control the default state of the event severity filter on the cluster overview page.
Previously, the "Only warnings" filter defaulted to
true, hiding normal events by default. This PR allows administrators to configure this default behavior globally via the backend CLI or Helm chart, improving visibility for users who prefer to see all events initially.The implementation preserves user preference: if a user has manually toggled the filter, their choice is saved in
localStorageand takes precedence over this global configuration.Related Issue
Fixes #4566
Changes
Backend:
FiltersWarningsOnlyfield to Config and HeadlampConfig structs.-filters-warnings-only(default:trueto maintain backward compatibility)./configAPI endpoint to expose this setting to the frontend.Frontend:
filtersWarningsOnly.Helm Chart:
config.events.warningsOnlyto values.yaml.HEADLAMP_CONFIG_FILTERS_WARNINGS_ONLYenvironment variable in deployment.yaml.Steps to Test
Test Case 1: Default Behavior (Backward Compatibility)
localStoragekeyEVENT_WARNING_SWITCH_FILTER_STORAGE_KEY).Test Case 2: Configured to Show All Events
config.events.warningsOnly: false(or run locally withgo run ./cmd/... -filters-warnings-only=false).localStoragekeyEVENT_WARNING_SWITCH_FILTER_STORAGE_KEYto simulate a fresh user session.Screenshots (if applicable)
(Optional: You can attach a screenshot here showing the toggle in the OFF state by default)
Notes for the Reviewer
true, ensuring no change in behavior for existing deployments unless explicitly configured.