[OLD] Fix rename cluster duplicate name issue / improve cluster metadata for UI #2944
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
dosubot
bot
added
the
size:S
vyncent-t
added
frontend
dosubot
bot
added
the
size:S
|
Backend Code coverage changed from 65.0% to 65.2%. Change: .2% 😃. Coverage report |
vyncent-t
force-pushed
the
improve-cluster-metadata-for-ui
branch
from
280613c to
f12841b
Compare
dosubot
bot
added
size:M
|
Backend Code coverage changed from 65.0% to 65.2%. Change: .2% 😃. Coverage report |
vyncent-t
changed the title
vyncent-t
changed the title
skoeva
reviewed
Feb 25, 2025
vyncent-t
force-pushed
the
improve-cluster-metadata-for-ui
branch
from
f12841b to
38a7582
Compare
|
Backend Code coverage changed from 65.0% to 65.2%. Change: .2% 😃. Coverage report |
|
@vyncent-t can you please add How to test to the PR description? |
joaquimrocha
requested changes
Feb 28, 2025
| <Typography>{t('translation|Name')}</Typography> | ||
| {displayOriginalName && ( | ||
| <Typography variant="body2" color="textSecondary"> | ||
| {t('translation|Original name of cluster: {{ originalName }}', { |
There was a problem hiding this comment.
Just Original name: ... should be enough given the context. Reads too verbose otherwise.
Comment on lines
249
to
250
| const sourcePath = cluster.meta_data?.origin?.kubeconfig; | ||
| const kubeconfigPath = process.env.KUBECONFIG ?? sourcePath; |
There was a problem hiding this comment.
I think it should always be the meta_data case here. No the KUBECONFIG env.
| return path | ||
| }(), | ||
| }, | ||
| "originalName": context.Name, |
There was a problem hiding this comment.
I see in the /config requests from the frontend that the original name is the one being sent as the name above, so we already have the original name + custom name (in the extensions) in the frontend AFAIU.
There was a problem hiding this comment.
in the frontend when I log the end result of a cluster that has been fetched, if it has been renamed then the cluster.name is set to the customName
looking at the /config response looks like the top name is the original name but after its processed and finished for the the frontend the cluster log information is different
so I think we do have access to the original name as long as it is never renamed
There was a problem hiding this comment.
I need to understand what "after its processed and finished for the the frontend " means in this case.
Seems like we likely need to always keep the original name as the key (till we also add the kubeconfig path) for both the backend and frontend, and use the renamed name just for the display.
backend/cmd/headlamp.go
Outdated
Comment on lines
1183
to
1184
| path, _ := c.getKubeConfigPath(context.SourceStr()) | ||
| return path |
There was a problem hiding this comment.
I think the path should be set from the methods that read the cluster in the first place rather than trying to find it again here.
There was a problem hiding this comment.
moved it so that we aren't trying to find it right inside of where it is being initialized
vyncent-t
force-pushed
the
improve-cluster-metadata-for-ui
branch
from
38a7582 to
2eed250
Compare
|
Backend Code coverage changed from 65.1% to 65.2%. Change: .1% 😃. Coverage report |
vyncent-t
force-pushed
the
improve-cluster-metadata-for-ui
branch
2 times, most recently
from
108d28b to
bdaa350
Compare
|
Backend Code coverage changed from 64.9% to 65.1%. Change: .2% 😃. Coverage report |
joaquimrocha
requested changes
Mar 7, 2025
backend/cmd/headlamp.go
Outdated
| @@ -1191,6 +1191,11 @@ func (c *HeadlampConfig) getClusters() []Cluster { | |||
| continue | |||
| } | |||
|
|
|||
| kubeconfigPath, err := c.getKubeConfigPath(context.SourceStr()) | |||
There was a problem hiding this comment.
The actual path of the kubeconfig needs to be corrected. This will always give the kubeconfig we loaded headlamp with, but if we have more than one, it's not clear this will yield the correct value. So I think that the way to add it is in the methods where we load the actual kubeconfig: we should add a reference to the file's path for each cluster.
There was a problem hiding this comment.
so to confirm, we find where we load the cluster information from the kube config, create a middle func that adds the file path from that kubeconfig, and attach it to the cluster information before sending it off again?
should the same be done for the original name then? and just save the name under clusterID or something similar before sending it off?
| return path | ||
| }(), | ||
| }, | ||
| "originalName": context.Name, |
There was a problem hiding this comment.
I need to understand what "after its processed and finished for the the frontend " means in this case.
Seems like we likely need to always keep the original name as the key (till we also add the kubeconfig path) for both the backend and frontend, and use the renamed name just for the display.
vyncent-t
force-pushed
the
improve-cluster-metadata-for-ui
branch
from
bdaa350 to
935b77a
Compare
|
Backend Code coverage changed from 65.0% to 65.1%. Change: .1% 😃. Coverage report |
|
updated the branch
|
dosubot
bot
added
size:L
vyncent-t
force-pushed
the
improve-cluster-metadata-for-ui
branch
from
e666e61 to
ab90e50
Compare
dosubot
bot
added
size:XL
vyncent-t
force-pushed
the
improve-cluster-metadata-for-ui
branch
4 times, most recently
from
4b7ac0d to
22660bd
Compare
|
Backend Code coverage changed from 64.9% to 65.2%. Change: .3% 😃. Coverage report |
vyncent-t
changed the title
vyncent-t
force-pushed
the
improve-cluster-metadata-for-ui
branch
from
22660bd to
2901cd6
Compare
|
Backend Code coverage changed from 64.9% to 65.2%. Change: .3% 😃. Coverage report |
joaquimrocha
requested changes
Mar 24, 2025
backend/pkg/kubeconfig/kubeconfig.go
Outdated
Comment on lines
48
to
49
| PathName string `json:"pathName"` | ||
| PathID string `json:"pathID"` |
There was a problem hiding this comment.
Instead of path name and path ID, which are confusing, why not kubeConfigPath and ID or ClusterID?
There was a problem hiding this comment.
will change these to kubeConfigPath and ClusterID
Comment on lines
+1516
to
+1517
| // note: we need to have it in a const for the linting to pass | ||
| const sourceKubeconfig = "kubeconfig" |
There was a problem hiding this comment.
This existed already before, right? I am surprised this change is needed.
There was a problem hiding this comment.
This one was a weird linter issue, it wouldn't accept it in the linter as an if block so I made some adjustments to it here that satisfy the linter while also being able to use it "correctly"
vyncent-t
force-pushed
the
improve-cluster-metadata-for-ui
branch
from
2901cd6 to
cd65438
Compare
|
Backend Code coverage changed from 65.0% to 65.2%. Change: .2% 😃. Coverage report |
… pathID Signed-off-by: Vincent T <[email protected]>
Signed-off-by: Vincent T <[email protected]>
…clusters Signed-off-by: Vincent T <[email protected]>
…d updates test Signed-off-by: Vincent T <[email protected]>
Changes Include SettingsCluster.tsx: - handles error logic from backend to prevent users from entering a name/custom name already in use by another cluster - introduces new confirm dialog for when users enter clashing names ConfirmDialog.tsx: - introduces optional params for disabling the Yes and No button, allowing for single option "continue" dialog clusterApi.ts: - reworks the current renameCluster function to now handle renaming for dynamic and non dynamic clusters differently Signed-off-by: Vincent T <[email protected]>
vyncent-t
force-pushed
the
improve-cluster-metadata-for-ui
branch
from
cd65438 to
edf8bd0
Compare
|
Backend Code coverage changed from 64.9% to 65.2%. Change: .3% 😃. Coverage report |
|
Unknown CLA label state. Rechecking for CLA labels. Send feedback to sig-contributor-experience at kubernetes/community. /check-cla |
|
k8s-ci-robot
added
the
cncf-cla: no
k8s-ci-robot
added
the
needs-rebase
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
vyncent-t
changed the title
k8s-ci-robot
added
the
size/XL
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.
This branch replaces the need for #2747
Description
This PR is needed as a part to fix the delete non dynamic cluster PR
originalNameorigin.kubeconfigandpathIDsourcefield in the home view to have an accurate path for kubeconfigsThis PR also fixes an issue with duplicate name clusters.
Currently, users are able to 'reuse' cluster names, causing the existing cluster to be replaced with the renamed one. This UI redirection provides a way to prevent this from happening from within headlamp only.
renameClusterfunctionality in the backend to cover both non dynamic and dynamic clustersChanges include
Cluster info
cluster.metadata.origin.kubeconfigandcluster.metadata.originalNameandcluster.metadata.pathIDHome View
Originnow displays correct Kubeconfig pathCluster Settings
OriginalNamenow displayed in non-dynamic cluster settingsHow to test
Test the source path
Test the original name label
NamesectionTest the duplicate rename blocker UI