app: home: Add delete button for clusters #2567
Draft
+525
−79
Conversation
This file contains 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
vyncent-t
added
enhancement
vyncent-t
force-pushed
the
app-cluster-delete-button
branch
from
c239834 to
1e40675
Compare
|
last push: re worked the buttons components so that the app does not fail at main screen, repushed current WIP removing the clusterAPI changes that caused failures working on better way to use delete cluster that deletes kubeconfig parts |
vyncent-t
force-pushed
the
app-cluster-delete-button
branch
2 times, most recently
from
59d4961 to
7fd77f4
Compare
vyncent-t
commented
Nov 19, 2024
vyncent-t
changed the title
dosubot
bot
added
the
size:M
vyncent-t
force-pushed
the
app-cluster-delete-button
branch
from
7fd77f4 to
9d3e8cd
Compare
dosubot
bot
added
size:L
vyncent-t
force-pushed
the
app-cluster-delete-button
branch
from
9d3e8cd to
1a6bb4d
Compare
vyncent-t
force-pushed
the
app-cluster-delete-button
branch
2 times, most recently
from
a248958 to
0fe190e
Compare
|
We have to be careful here with calling this "Delete cluster" What it is doing more specifically is "deleting the cluster entry in the .kube/config file, and then any context entries that are related to that cluster". Not sure about the second part... but if it isn't it probably should do that. Because those contexts won't make sense anymore with the cluster gone that they were using. |
Maybe we call it "remove"? Would that make a difference? |
joaquimrocha
requested changes
Dec 10, 2024
| @@ -9,7 +11,8 @@ import { DialogTitle } from './Dialog'; | |||
|
|
|||
| export interface ConfirmDialogProps extends MuiDialogProps { | |||
| title: string; | |||
| description: string; | |||
| description: string | JSX.Element; | |||
There was a problem hiding this comment.
Maybe you want ReactNode.
Comment on lines
65
to
68
| {t('Cancel')} | ||
| </Button> | ||
| <Button disabled={!checkboxClicked} onClick={onConfirmationClicked} color="primary"> | ||
| {t('I Agree')} |
There was a problem hiding this comment.
I know I asked for "I agree", but I didn't remember this was in the context of an existing ConfirmDialog.
The ConfirmDialog should work similar whether you have a checkbox or not. I think it's better to add the checkbox (when existing) to the original code, rather than having two different ways of rendering the dialog. And the buttons should remain with the original labels, unless we want to provide properties for those too.
There was a problem hiding this comment.
Done! made some changes to the logic for the dialog which should allow us to just provide a description and then the smaller section of that description and the checkbox should render
vyncent-t
force-pushed
the
app-cluster-delete-button
branch
from
0fe190e to
7e3f7bd
Compare
|
Backend Code coverage changed from 61.0% to 60.7%. Change: -.3% 😞. Coverage report |
vyncent-t
force-pushed
the
app-cluster-delete-button
branch
from
7e3f7bd to
dc7cf04
Compare
|
last push fixes format issue |
|
Backend Code coverage changed from 61.0% to 60.8%. Change: -.2% 😞. Coverage report |
vyncent-t
force-pushed
the
app-cluster-delete-button
branch
from
490d106 to
049c2e0
Compare
|
Backend Code coverage changed from 61.0% to 60.7%. Change: -.3% 😞. Coverage report |
|
Backend Code coverage changed from 61.1% to 60.7%. Change: -.4% 😞. Coverage report |
vyncent-t
force-pushed
the
app-cluster-delete-button
branch
2 times, most recently
from
9037df3 to
95c86fd
Compare
|
Backend Code coverage changed from 61.1% to 60.7%. Change: -.4% 😞. Coverage report |
|
had to manually resolve merge conflict in this tab as the pushes I made after already resolving locally the normal way would not get read even after rebasing |
|
Backend Code coverage changed from 61.1% to 60.7%. Change: -.4% 😞. Coverage report |
vyncent-t
force-pushed
the
app-cluster-delete-button
branch
from
c490d0f to
61ab7a2
Compare
|
Backend Code coverage changed from 61.1% to 60.7%. Change: -.4% 😞. Coverage report |
illume
reviewed
Jan 3, 2025
| marginTop: '10px', | ||
| }} | ||
| > | ||
| <DialogContentText id="alert-dialog-description"> |
There was a problem hiding this comment.
This id is not unique. Is it used for anything?
There was a problem hiding this comment.
this is from the current main so I'm not sure if this is named that way for a reason
backend/cmd/headlamp.go
Outdated
| @@ -1365,6 +1377,35 @@ func (c *HeadlampConfig) addContextsToStore(contexts []kubeconfig.Context, setup | |||
| return setupErrors | |||
| } | |||
|
|
|||
| // removeContextFromFile removes the context from the kubeconfig file. | |||
| func removeContextFromFile(w http.ResponseWriter, contextName string) error { | |||
There was a problem hiding this comment.
Please add a test for this?
| import { useTranslation } from 'react-i18next'; | ||
| import { DialogTitle } from './Dialog'; | ||
|
|
||
| export interface ConfirmDialogProps extends MuiDialogProps { | ||
| title: string; | ||
| description: ReactNode; | ||
| description: string | React.ReactNode; | ||
| checkboxDescription?: string; |
There was a problem hiding this comment.
Please document this field?
(If you can document the other ones whilst you're here... that would be nice to have).
There was a problem hiding this comment.
@vyncent-t I know we talked about adding this prop but in the end I don't think this is too specific a behavior to have in the component. Maybe we can pass a fragment that includes the description and the checkbox as part of the description. I will comment below why we don't need to make Agree insensitive depending on the check mark.
| aria-labelledby="alert-dialog-title" | ||
| aria-describedby="alert-dialog-description" | ||
| > | ||
| <DialogTitle id="alert-dialog-title">{title}</DialogTitle> | ||
| <DialogContent ref={focusedRef}> | ||
| <DialogContentText id="alert-dialog-description">{description}</DialogContentText> | ||
| {props.checkboxDescription && ( |
There was a problem hiding this comment.
Can you please add a story for the new states?
| @@ -78,7 +78,8 @@ export async function setCluster(clusterReq: ClusterRequest) { | |||
| // @todo: needs documenting. | |||
|
|
|||
| export async function deleteCluster( | |||
| cluster: string | |||
| cluster: string, | |||
| removeKubeConfig?: boolean | |||
There was a problem hiding this comment.
Can you please document this?
backend/cmd/headlamp.go
Outdated
| @@ -1365,6 +1377,35 @@ func (c *HeadlampConfig) addContextsToStore(contexts []kubeconfig.Context, setup | |||
| return setupErrors | |||
| } | |||
|
|
|||
| // removeContextFromFile removes the context from the kubeconfig file. | |||
| func removeContextFromFile(w http.ResponseWriter, contextName string) error { | |||
There was a problem hiding this comment.
removeContextFromFile could be removeContextFromDefaultKubeConfig? To be more descriptive. With just File it wasn't obvious that it was acting on the defaultKubeConfigPersistenceFile.
backend/cmd/headlamp.go
Outdated
| @@ -1365,6 +1377,35 @@ func (c *HeadlampConfig) addContextsToStore(contexts []kubeconfig.Context, setup | |||
| return setupErrors | |||
| } | |||
|
|
|||
| // removeContextFromFile removes the context from the kubeconfig file. | |||
| func removeContextFromFile(w http.ResponseWriter, contextName string) error { | |||
There was a problem hiding this comment.
What happens when people have multiple kubeconfig files?
vyncent-t
force-pushed
the
app-cluster-delete-button
branch
from
61ab7a2 to
dedd5f5
Compare
|
Backend Code coverage changed from 63.5% to 63.3%. Change: -.2% 😞. Coverage report |
Signed-off-by: Vincent T <[email protected]>
vyncent-t
force-pushed
the
app-cluster-delete-button
branch
2 times, most recently
from
87718ea to
f09906b
Compare
dosubot
bot
added
size:XL
Signed-off-by: Vincent T <[email protected]>
Signed-off-by: Vincent T <[email protected]>
Signed-off-by: Vincent T <[email protected]>
…onfig Signed-off-by: Vincent T <[email protected]>
vyncent-t
force-pushed
the
app-cluster-delete-button
branch
from
f09906b to
4ec545d
Compare
|
Backend Code coverage changed from 63.5% to 63.0%. Change: -.5% 😞. Coverage report |
|
still very new to Go, let me know if is anything that I need to change Summary so farI have expanded on the changes for the deleteCluster function to now try to remove the cluster by context name in scenarios that use multi config files in the same dir. The linter did not like how long the deleteCluster function was starting to get so I added an additional function
troubleshooting notesI was able to get the live delete test working (I would just create a test kube called deltakube to match what is already here for the test) but for it to work it required changes to some environment variables that also caused waterfall issues with other tests when running make backend-test, it also was not compatible with the current workflows we have for github, which is why I am trying to mock the env now for the Currently working onI am still trying to create a reasonable test for the If I can make the test small enough I will change the current kubeconfig_remove to just hold the config info we want to delete (the delta one) and have the kubeconfig_rename and kubeconfig_remove act as the multiple config set up in that temp test env) I would also likely need to have that test find the name of the path where the contextName is found and check it again to see if that has been removed, but this leads into some new questions: should I rewrite the multi config use func for example: and should I further refactor the deleteCluster function to try to delete a cluster by a more unique identifier other than just the name? I want to avoid having a scenario where I want to delete deltakube from configB but since configA is ran first it deletes it there (unless its not possible to have the same name even on different config files but I think you can with no issue) any thoughts? @illume |
joaquimrocha
requested changes
Jan 8, 2025
There was a problem hiding this comment.
Left some comments. I think that before this PR, for to make it much simpler, the config for the clusters in our backend needs to contain the kubeconfig if they are loaded from one. I know it's a different case/PR, but without that, these changes are complex and can contain corner cases that are unexpected.
| @@ -234,6 +234,18 @@ func serveWithNoCacheHeader(fs http.Handler) http.HandlerFunc { | |||
| } | |||
| } | |||
|
|
|||
| // defaultKubeConfigFile returns the default path to the kubeconfig file. | |||
| func defaultKubeConfigFile() (string, error) { | |||
There was a problem hiding this comment.
AFAIU, we have GetDefaultKubeConfigPath() in the config.go, we also have a way to retrive this from k8s client-go.
|
|
||
| if removeKubeConfig { | ||
| // delete context from actual default kubecofig file | ||
| kubeConfigFile, err := defaultKubeConfigFile() |
There was a problem hiding this comment.
I think clusters can be loaded from multiple kubeconfig files, yet we only have a flag telling us whether the clusters from kubeconfig or dynamic. This is a limitation in our backend code (not your PR), but one that we have to handle before we consider this PR IMO.
Tied to this is the notion of what happens when we load a cluster with the same name from a different (or same) file.
| import { useTranslation } from 'react-i18next'; | ||
| import { DialogTitle } from './Dialog'; | ||
|
|
||
| export interface ConfirmDialogProps extends MuiDialogProps { | ||
| title: string; | ||
| description: ReactNode; | ||
| description: string | React.ReactNode; |
There was a problem hiding this comment.
You don't need to explicitly make it a string, as it's a type already covered by ReactNode.
| import { useTranslation } from 'react-i18next'; | ||
| import { DialogTitle } from './Dialog'; | ||
|
|
||
| export interface ConfirmDialogProps extends MuiDialogProps { | ||
| title: string; | ||
| description: ReactNode; | ||
| description: string | React.ReactNode; | ||
| checkboxDescription?: string; |
There was a problem hiding this comment.
@vyncent-t I know we talked about adding this prop but in the end I don't think this is too specific a behavior to have in the component. Maybe we can pass a fragment that includes the description and the checkbox as part of the description. I will comment below why we don't need to make Agree insensitive depending on the check mark.
Comment on lines
+81
to
+84
| {props.checkboxDescription ? ( | ||
| <Button disabled={!checkedChoice} onClick={onConfirmationClicked} color="primary"> | ||
| {t('I Agree')} | ||
| </Button> |
There was a problem hiding this comment.
This behavior is leading to the following problem: if I want to remove the cluster from Headlamp but not delete it from the kubeconfig (which was the point of the checkbox), then I can not even remove the cluster Headlamp (as the agree button is disabled).
| }} | ||
| title={t('translation|Delete Cluster')} | ||
| description={t( | ||
| 'translation|Are you sure you want to remove the cluster "{{ clusterName }}"?', | ||
| 'translation|This action will delete cluster "{{ clusterName }}" from {{ source }}.', |
There was a problem hiding this comment.
Let's make this about the kubeconfig. If a plugin added the cluster, or we loaded it from a file directly, we just want to confirm that the user wants to remove the cluster. The removal from kubeconfig (i.e. non-Headlamp kubeconfigs) should be a configurable option for the user as a checkbox, and that's a very particular case.
| @@ -1377,6 +1377,126 @@ func (c *HeadlampConfig) addContextsToStore(contexts []kubeconfig.Context, setup | |||
| return setupErrors | |||
| } | |||
|
|
|||
| // collectMultiConfigPaths looks at the default dynamic directory | |||
There was a problem hiding this comment.
This commit should be merged with the previous one AFAIU.
| "No": "Nein", | ||
| "I Agree": "", |
There was a problem hiding this comment.
Please merge with the commit that adds the strings.
knrt10
requested changes
Jan 9, 2025
There was a problem hiding this comment.
Nice work @vyncent-t. Some suggestions. Also, the code coverage is decreasing for backend, please add more tests for functions added.
| preferences: {} | ||
| clusters: | ||
| - cluster: | ||
| certificate-authority-data: LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSUMvakNDQWVhZ0F3SUJBZ0lCQURBTkJna3Foa2lHOXcwQkFRc0ZBREFWTVJNd0VRWURWUVFERXdwcmRXSmwKY201bGRHVnpNQjRYRFRJeU1EZ3lOakV4TURRMU0xb1hEVE15TURneU16RXhNRFExTTFvd0ZURVRNQkVHQTFVRQpBeE1LYTNWaVpYSnVaWFJsY3pDQ0FTSXdEUVlKS29aSWh2Y05BUUVCQlFBRGdnRVBBRENDQVFvQ2dnRUJBTnk3Ci9kREMxV0w3TXNSWGV2Z2tUQXkzcFZHMVVLa1VQeXd4cS9ETHBPdmRzQmloQjZoVmN1bWNZUTkzYUxLbERzSXMKR0Q0QUJkUFM4cEFPMzhMb3RBWWVDeDIwcDFPem9LYVMvVkp6ZlJKQWVUSStCY3dzRjh2U1VXYU0reWZ4STBPUgpnalE0OVR0eUppYURyS2tzbnd4R3Y0K0U3aWFhZUVPMG55U01EcnpON1RvYkVyb1pObHRzNkdMN2tpTDB0TG5ZCkorNnNtSHlhSGh6WThaR0JZMFdWUXpzNENFMnJ0Q1k5eTV4N2F3bUlDUWE2anBXVFVQazNqa0RMcU93bEQyRmMKcHNkeXI4a1Z3UUhTUUVnRkg2Yzgwdnp3Ny9RSUVDdGRYNlZRRnE1bzYzOWlvc3hQcXVKV3ZtMGVjdkx5dC81cApxNXZpNzMxWThEb0VDMjFtS2NzQ0F3RUFBYU5aTUZjd0RnWURWUjBQQVFIL0JBUURBZ0trTUE4R0ExVWRFd0VCCi93UUZNQU1CQWY4d0hRWURWUjBPQkJZRUZJNFFkU2FSRFVodi8wWjk0ZzV5RmlVdWlMZHBNQlVHQTFVZEVRUU8KTUF5Q0NtdDFZbVZ5Ym1WMFpYTXdEUVlKS29aSWh2Y05BUUVMQlFBRGdnRUJBQ3pWWUpBUzQ1UFBOSFVSaDJKWQpKWDFycmFMdGNTbzVuNG1DVy9oeE5YdHpCMlIzWkhPU0hnNmF2R3JNeFY4ZlpCdmtBdEJFaUYzM2JvRThzZzVhCjhhWHRFTjR5bzlZQ2FZc2ZXK2tNNlZDRUdtVWd5bm13aXltYTBzSW5USlZ1R3ZVbDVucVhjUHJJdW9OTVVrTUwKdCsrckxCb0NwY2xrN09VSTA0dXZvanpxc2hsQ0JiMURSOXRwT0s0Kys0UGdPait6OXZ0N3g0dzhMYlhvQmtvegozOEJyVEoyQ3NqbU0xS2ZqZXlpNWdHVmFjeE9YSXRjbXprNzRpQzZ0SjdqVm1MVmNacEc5ZElvcFk5WTBaTkQ0CmQzZjlmOGdCWkJzaXA0a3gxMmFxMlJ5dzFYNGVOaFY2dW5OaCtHVHNhNlFDSlJ0Zk9FK1Q4Njd2ZHlPbjZMb2wKYWQ4PQotLS0tLUVORCBDRVJUSUZJQ0FURS0tLS0tCg== |
There was a problem hiding this comment.
Please change it simple base64 encoded value. Using actual certificate data in test files, even if they are expired or invalid, is not a best practice from a security perspective. I think we should update them in other places as well like https://github.com/headlamp-k8s/headlamp/blob/main/backend/pkg/kubeconfig/test_data/kubeconfig_partialcontextvalid#L31.
| users: | ||
| - name: minikubetest | ||
| user: | ||
| client-certificate-data: LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSURJekNDQWd1Z0F3SUJBZ0lJZlJpZk1qZWl1eFV3RFFZSktvWklodmNOQVFFTEJRQXdGVEVUTUJFR0ExVUUKQXhNS2EzVmlaWEp1WlhSbGN6QWVGdzB5TWpBNE1qWXhNVEEwTlROYUZ3MHlOREF4TURJeE1ESXhNelZhTURZeApGekFWQmdOVkJBb1REbk41YzNSbGJUcHRZWE4wWlhKek1Sc3dHUVlEVlFRREV4SmtiMk5yWlhJdFptOXlMV1JsCmMydDBiM0F3Z2dFaU1BMEdDU3FHU0liM0RRRUJBUVVBQTRJQkR3QXdnZ0VLQW9JQkFRQzRLUmwrS0lsN0NJYVgKbzIwYjdBOVEvaURDbUN6dWFXMSs3WEJyelhiZHNmNkNaRzhVMWZYWTdVWXl3bXVhYkZldUFrUHRBT1hyWVg0YQpMTGZtWTRvdkZYc1RQWmtPUktJeWRFUmNnLy9hOStPd3d2c1ZCUUp4NFplbUtrN1NzaFYxcjl3WGVqVnJIUkFOCm5xQ3JIQVhFNHA5bmFKZHNkTXIyQWdDa0VIK01tTFNqTExNL1lWcnExdmJpRWRtUVFSWHduVnFwcmNyRXBIQzUKWWJJenl4cVZRWWZIZVdWc2N0SUxFeVdPMFQwMS9tYkZ2RVY4QW9BL3phekIycjF3Y0VaeUNSRXFXbExrS2RXTwpNYmU1WnlwMDNhQzlBOSs4cThQNFBEOUNnVXlrcVovN0xydGlja2k0TVBsK2VmaGFlUk9YSEJMSURuQmplTHJkCmJGdHpaOVhKQWdNQkFBR2pWakJVTUE0R0ExVWREd0VCL3dRRUF3SUZvREFUQmdOVkhTVUVEREFLQmdnckJnRUYKQlFjREFqQU1CZ05WSFJNQkFmOEVBakFBTUI4R0ExVWRJd1FZTUJhQUZJNFFkU2FSRFVodi8wWjk0ZzV5RmlVdQppTGRwTUEwR0NTcUdTSWIzRFFFQkN3VUFBNElCQVFBRjNvaTFvNVlNM1UvOWxPRElhaUpmaGllNzdieG1pN3NwCitiL0NEOGRCTXhIdWpPVnBSaTFNaHRJa2U3N2U1RVVuZEFGRzYvQTQwK3c2TGtCYXJFUEl5R2daRlBvZkttcSsKRGlIMGxPZHBYY0hFd3laTjhWSmdRd0JKUkhKcDhBc0p3TGFYWGplU1FQdmZyeHhLdUFGenRzeXNaYlBMUkxoYQpjeXZmeDNwTE91ZVJ4MDJqQVZUUlNJUGNPZEV4SERPa0FGWFFCdDV4TFo2eGFKTU1VQjZXNUYwcVpPelFuVUZsCk80QUNNOEhnOEdKc2xqLzFqZnpZaGlneWdwL2psQ0Jkd1Izb2c1ZXFqaC9ZRzlxWHVsU2Z0WUNhMURaOEp2QnAKaGRSYzZxOVM0ZFdtRW9zMmkxTDA1WUs3ZFBaQk5JVHRLNkVzQS9CRCs0VlVWRHczZldkNQotLS0tLUVORCBDRVJUSUZJQ0FURS0tLS0tCg== |
| // collectMultiConfigPaths looks at the default dynamic directory | ||
| // (e.g. ~/.config/Headlamp/kubeconfigs) and returns any files found there. | ||
| // This is called from the 'else' block in deleteCluster(). | ||
| func (c *HeadlampConfig) collectMultiConfigPaths() ([]string, error) { |
There was a problem hiding this comment.
This should be moved to backend/pkg/config/config.go
| return nil, fmt.Errorf("getting default kubeconfig persistence dir: %w", err) | ||
| } | ||
|
|
||
| entries, err := os.ReadDir(dynamicDir) |
There was a problem hiding this comment.
Currently, the function reads all files from the directory without checking their extensions or format. Can you please add validation for known kubeconfig extensions?
| return nil, fmt.Errorf("reading dynamic kubeconfig directory: %w", err) | ||
| } | ||
|
|
||
| var configPaths []string //nolint:prealloc |
There was a problem hiding this comment.
please remove this linter and allocate it earlier
| err, | ||
| "getting default kubeconfig persistence file", | ||
| ) | ||
| http.Error(w, "getting default kubeconfig persistence file", http.StatusInternalServerError) |
There was a problem hiding this comment.
please do not http.Error here. This should be done at the top level function. This function should return only error message. This could lead to double error handling if the caller also tries to handle the error. Please remove this.
| // removeContextFromConfigs does the real iteration over the configPaths. | ||
| func removeContextFromConfigs(w http.ResponseWriter, contextName string, configPaths []string) error { | ||
| var removed bool | ||
|
|
There was a problem hiding this comment.
Please check whether contextName is empty or if configPath exists
if contextName == "" {
return fmt.Errorf("context name cannot be empty")
}
if len(configPaths) == 0 {
return fmt.Errorf("no config paths provided")
}| err, "removing cluster from kubeconfig", | ||
| ) | ||
|
|
||
| http.Error(w, "removing cluster from kubeconfig", http.StatusInternalServerError) |
| e, | ||
| "context not found in any file", | ||
| ) | ||
| http.Error(w, e.Error(), http.StatusBadRequest) |
| @@ -665,6 +665,34 @@ func TestRenameCluster(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestRemoveContextFromDefaultKubeConfig(t *testing.T) { | |||
There was a problem hiding this comment.
This function should also be moved. Thanks
This was referenced Jan 14, 2025
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
Fixes #1418
This PR updates the cluster deletion functionality in Headlamp to allow users to delete non-dynamic clusters directly from the home page. Previously, only clusters added through Headlamp could be deleted, which led to confusion for users attempting to delete clusters originating from other sources (e.g., kubeconfig).
To address this, the deletion process has been updated to clearly inform users about the impact of deleting a cluster, particularly when the cluster originates from non-Headlamp sources.
Images
Changes Made
Steps to Test
Notes