Skip to content
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

Add enhancement to rename cluster #1974

Merged
merged 5 commits into from
Jul 22, 2024
Merged

Add enhancement to rename cluster #1974

merged 5 commits into from
Jul 22, 2024

Conversation

knrt10
Copy link
Contributor

@knrt10 knrt10 commented May 13, 2024

This adds feature where user can now use renamed cluster name in their UI.

Fixes: #1653

Testing

  • Remove old kubeconfig rm ~/.config/Headlamp/kubeconfigs/config
  • Run app cd app && npm run dev-only-app
  • Run backend with stateless clusters enabled cd backend && go run ./cmd -dev -enable-dynamic-clusters
  • Go to cluster settings and there is option for user to rename their cluster

Screenshots

Screenshot from 2024-05-13 12-49-05

Screenshot from 2024-05-13 12-49-20

TODO

  • Add for stateless clusters
  • Add tests

@knrt10 knrt10 force-pushed the renameCluster branch 4 times, most recently from 6e2ae1b to ff9fa30 Compare May 13, 2024 08:06
backend/cmd/headlamp.go Outdated Show resolved Hide resolved
@knrt10 knrt10 force-pushed the renameCluster branch 2 times, most recently from feffd6d to 930b20d Compare May 14, 2024 05:46
@illume illume marked this pull request as draft May 17, 2024 07:42
@knrt10 knrt10 force-pushed the renameCluster branch 3 times, most recently from 128746c to a8c6172 Compare May 20, 2024 08:15
backend/cmd/headlamp.go Outdated Show resolved Hide resolved
@knrt10 knrt10 force-pushed the renameCluster branch 2 times, most recently from d7f04c6 to e440342 Compare May 28, 2024 07:29
@joaquimrocha
Copy link
Collaborator

So I swear I had sent a message here with some screenshots showing some problems but I don't see the message now...
Anyway, I pushed some changes to make the UI hopefully nicer + restrict it to electron only (this is important!), and I have also verified that some of the problems I had which made me think we shouldn't add this to the 0.24.0 release were in a past iteration of this branch.
Now things seem to work fine, though it looks like if I switch between the default cluster name and the new one, in the settings it shows me the previous name (not sure if this is autocomplete getting in the way).

Anyway, here is how it looks now (notice how the error status no longer extends the input till the end of the space):
Screenshot

@joaquimrocha
Copy link
Collaborator

@illume What do you think? Should try to add it for 0.24.0 or should we move it (keep it now, since I had changed the milestone when I was in the outdated branch) to 0.25.0?

@knrt10 knrt10 force-pushed the renameCluster branch 3 times, most recently from 113c91b to 656e6e3 Compare June 3, 2024 14:39
frontend/src/i18n/locales/pt/translation_old.json Outdated Show resolved Hide resolved
@knrt10 knrt10 force-pushed the renameCluster branch 3 times, most recently from 5d3d701 to bc1a488 Compare July 15, 2024 08:24
@knrt10 knrt10 requested a review from joaquimrocha July 15, 2024 08:25
@knrt10 knrt10 force-pushed the renameCluster branch 2 times, most recently from 8b00cd3 to c525234 Compare July 15, 2024 09:13
@joaquimrocha joaquimrocha added enhancement New feature or request frontend Issues related to the frontend labels Jul 18, 2024
@joaquimrocha joaquimrocha added this to the v0.26.0 milestone Jul 18, 2024
Copy link
Collaborator

@illume illume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 thanks

@illume illume added the backend Issues related to the backend label Jul 18, 2024
@illume
Copy link
Collaborator

illume commented Jul 18, 2024

@knrt10

Seems the text is wrong after I change it, then change it back.

  • click 'minikube' cluster and cluster settings
  • change 'minikube' to 'mycluster'
  • change back to 'minikube'
  • click cluster, and cluster settings... it says 'mycluster' still in the input text.

See in the minikube cluster but the text has the old 'mycluster' there. It should be 'minikube' or empty.
image

illume
illume previously requested changes Jul 18, 2024
Copy link
Collaborator

@illume illume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a look around for the bug a little bit, but couldn't find it.

Probably we should add some tests, and perhaps refactor SettingsCluster a bit.

@illume
Copy link
Collaborator

illume commented Jul 18, 2024

Maybe we should add this flag to dev mode? Or to the app?

diff --git a/Makefile b/Makefile
index 2d426825..102fd637 100644
--- a/Makefile
+++ b/Makefile
@@ -86,10 +86,10 @@ run-backend:
        @echo "**** Warning: Running with Helm and dynamic-clusters endpoints enabled. ****"

 ifeq ($(UNIXSHELL),true)
-       HEADLAMP_BACKEND_TOKEN=headlamp HEADLAMP_CONFIG_ENABLE_HELM=true HEADLAMP_CONFIG_ENABLE_DYNAMIC_CLUSTERS=true ./backend/headlamp-server -dev -proxy-urls https://artifacthub.io/*
+       HEADLAMP_BACKEND_TOKEN=headlamp HEADLAMP_CONFIG_ENABLE_HELM=true HEADLAMP_CONFIG_ENABLE_DYNAMIC_CLUSTERS=true ./backend/headlamp-server -dev -proxy-urls https://artifacthub.io/* -enable-dynamic-clusters
 else
        @echo "**** Running on Windows without bash or zsh. ****"
-       @cmd /c "set HEADLAMP_BACKEND_TOKEN=headlamp&& set HEADLAMP_CONFIG_ENABLE_HELM=true&& set HEADLAMP_CONFIG_ENABLE_DYNAMIC_CLUSTERS=true&& backend\headlamp-server -dev -proxy-urls https://artifacthub.io/*"
+       @cmd /c "set HEADLAMP_BACKEND_TOKEN=headlamp&& set HEADLAMP_CONFIG_ENABLE_HELM=true&& set HEADLAMP_CONFIG_ENABLE_DYNAMIC_CLUSTERS=true&& backend\headlamp-server -dev -proxy-urls https://artifacthub.io/*" -enable-dynamic-clusters
 endif

 run-frontend:

knrt10 and others added 5 commits July 22, 2024 08:51
This adds route that handles logic to rename clusters. When user updates
their cluster name from UI, it sends the customName and source of the
cluster where it is fetched from. The backend checks the source and
updates the context in the source. It also maps the new customName to
original name of cluster in the source.

Fixes: #1653
Signed-off-by: Kautilya Tripathi <ktripathi@microsoft.com>
This adds UI to rename cluster name in cluster settings component. This
does not update the values in kubeconfig/backend only for React state.

Signed-off-by: Kautilya Tripathi <ktripathi@microsoft.com>
Signed-off-by: Kautilya Tripathi <ktripathi@microsoft.com>
Signed-off-by: Kautilya Tripathi <ktripathi@microsoft.com>
Signed-off-by: Joaquim Rocha <joaquim.rocha@microsoft.com>
Signed-off-by: Kautilya Tripathi <ktripathi@microsoft.com>
@knrt10 knrt10 requested a review from illume July 22, 2024 03:41
@knrt10
Copy link
Contributor Author

knrt10 commented Jul 22, 2024

@illume fixed, can you please test again. Thanks

@knrt10 knrt10 dismissed stale reviews from illume and joaquimrocha July 22, 2024 03:44

rerequested

@illume
Copy link
Collaborator

illume commented Jul 22, 2024

I'm having an issue where it's not changing...

image

@illume
Copy link
Collaborator

illume commented Jul 22, 2024

Ok... it works after I clear my localstorage. I think when it was broken something bad got in there.

Copy link
Collaborator

@illume illume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. 👍

@illume illume requested a review from ashu8912 July 22, 2024 11:00
Copy link
Contributor

@skoeva skoeva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great on my end, thanks for this! 🙌

@illume illume merged commit a5654da into main Jul 22, 2024
19 checks passed
@illume illume deleted the renameCluster branch July 22, 2024 12:40
@illume illume mentioned this pull request Jul 25, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Issues related to the backend enhancement New feature or request frontend Issues related to the frontend
Projects
Development

Successfully merging this pull request may close these issues.

Option to rename a cluster?
5 participants