frontend: Enable cluster deletion in browser#4461
frontend: Enable cluster deletion in browser#4461alokdangre wants to merge 2 commits intokubernetes-sigs:mainfrom
Conversation
|
@illume ptal |
There was a problem hiding this comment.
Pull request overview
This PR enables cluster deletion in browser mode when dynamic cluster management is enabled in the backend configuration. Previously, cluster deletion was restricted to the Electron desktop app only.
Changes:
- Added
isDynamicClusterEnabledflag to Redux config state to track backend dynamic cluster support - Updated ClusterContextMenu to show delete option when either running in Electron or when dynamic clusters are enabled
- Modified backend token initialization to support development scenarios via environment variable
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| frontend/src/redux/configSlice.ts | Adds isDynamicClusterEnabled field to ConfigState and updates setConfig reducer to handle the new optional parameter |
| frontend/src/redux/configSlice.test.ts | Adds test case to verify setConfig properly handles isDynamicClusterEnabled parameter |
| frontend/src/helpers/getHeadlampAPIHeaders.ts | Initializes backendToken from REACT_APP_HEADLAMP_BACKEND_TOKEN environment variable for development scenarios |
| frontend/src/components/App/Home/ClusterContextMenu.tsx | Updates cluster delete menu item visibility logic to check isDynamicClusterEnabled in addition to isElectron() |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
illume
left a comment
There was a problem hiding this comment.
Thanks for this.
I see this issue in the filing GitHub check:
src/components/project/ProjectCreateFromYaml.stories.tsx:35:7 - error TS2741: Property 'isDynamicClusterEnabled' is missing in type '{ clusters: null; statelessClusters: null; allClusters: any; settings: { tableRowsPerPageOptions: number[]; timezone: string; useEvict: true; }; }' but required in type 'ConfigState'.
35 config: {
~~~~~~
src/redux/configSlice.ts:49:3
49 isDynamicClusterEnabled: boolean;
~~~~~~~~~~~~~~~~~~~~~~~
'isDynamicClusterEnabled' is declared here.
[error] Found 1 errors and 2 warningsPlease also consider the open comments?
da1ade9 to
9777483
Compare
|
@illume fixed the changes ptal |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
joaquimrocha
left a comment
There was a problem hiding this comment.
I am not sure we should make this dependent on the dynamic clusters being available because the dynamic clusters don't have much to do with being able to delete a cluster from the kubeconfig.
Still we should make sure that we do not offer this option (and especially that we do not effectively remove clusters) when Headlamp has been deployed with kubeconfig as an option. We need to be extremely cautious with this, as we don't want users removing clusters from their company-deployed Headlamp.
So maybe this means we should have a flag --allow-kubeconfig-removal or something, which would be false by default, but we'd set it to true when developing and when running the app (which would work in headless mode).
BTW, there was already this PR in draft from last year: #3339
that's very nice point, it seems good to me, im implementing the suggested review |
9777483 to
71c1809
Compare
71c1809 to
3ec736e
Compare
|
Changes look good. I was gonna suggest we use So to be clear: either we add a new "AllowKubeConfigChanges" flag, or we do not any new flags and allow deleting clusters from the kubeconfig files only when isElectron() is true. |
doesn't it already work like this? we can delete clusters in the app |
yes its the logic currently used in app, but i added a flag in this pr, so i will remove the flag from the electron app |
3ec736e to
7e55baf
Compare
There was a problem hiding this comment.
Thanks for making those changes. It got me thinking about something else...
Can we enable this flag by default in "headless" mode? See the app/electron/main.ts for headless mode.
Since this is a local thing, if we can enable this functionality for headless mode it will more closely match the abilities of the app. Since the security context of these two modes are pretty much the same... they are both app mode run by an individual.
7e55baf to
aeaed7e
Compare
done |
illume
left a comment
There was a problem hiding this comment.
Thanks! 🎉
(I’ll leave this one open to give the others a chance to have a look)
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alokdangre, illume The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Enable cluster deletion when running in a browser if dynamic cluster management is enabled in the backend. This was previously restricted to the Electron app.
Introduces a new backend configuration flag --allow-kubeconfig-removal to control whether users can delete clusters sourced from their kubeconfig via the UI. - backend: Add flag and expose it in client config. - frontend: Conditionally enable delete option based on flag. - electron: Enable flag in headless mode (shared security context). - makefile: Enable flag in dev mode.
aeaed7e to
2c04685
Compare
Summary
This change enables the deletion of clusters when running Headlamp in a browser, provided that dynamic cluster management is enabled in the backend configuration.
Previously, cluster deletion was restricted to the Electron app.
Related Issue
Fixes #3053
Changes
Steps to Test
Screenshots (if applicable)
Screen.Recording.2026-01-27.114635.mp4