-
Notifications
You must be signed in to change notification settings - Fork 233
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
frontend: Improve non dynamic clusters metadata for UI #2944
base: main
Are you sure you want to change the base?
Conversation
Backend Code coverage changed from 65.0% to 65.2%. Change: .2% 😃. Coverage report
|
280613c
to
f12841b
Compare
Backend Code coverage changed from 65.0% to 65.2%. Change: .2% 😃. Coverage report
|
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a couple of comments.
<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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just Original name: ...
should be enough given the context. Reads too verbose otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
const sourcePath = cluster.meta_data?.origin?.kubeconfig; | ||
const kubeconfigPath = process.env.KUBECONFIG ?? sourcePath; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should always be the meta_data case here. No the KUBECONFIG env.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
return path | ||
}(), | ||
}, | ||
"originalName": context.Name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
path, _ := c.getKubeConfigPath(context.SourceStr()) | ||
return path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved it so that we aren't trying to find it right inside of where it is being initialized
38a7582
to
2eed250
Compare
Backend Code coverage changed from 65.1% to 65.2%. Change: .1% 😃. Coverage report
|
108d28b
to
bdaa350
Compare
Backend Code coverage changed from 64.9% to 65.1%. Change: .2% 😃. Coverage report
|
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
bdaa350
to
935b77a
Compare
Backend Code coverage changed from 65.0% to 65.1%. Change: .1% 😃. Coverage report
|
updated the branch
console log clusters in the home view now looks like |
… pathID Signed-off-by: Vincent T <[email protected]>
Signed-off-by: Vincent T <[email protected]>
…clusters Signed-off-by: Vincent T <[email protected]>
…uster to a name already in use on that config file works: - the config path for renaming the config now uses a the accurate source path on user machines for backend logic (should work for multi config env) - the rename backend prevents renaming if the custom name from the request is already within the config file (either as a context name or a custom name) - add some UI to the frontend that responds when the isunique checker returns the http error Signed-off-by: Vincent T <[email protected]>
c891793
to
5f7791f
Compare
This branch replaces the need for #2747
Description
This PR aims to improve the cluster information available for clusters via the metadata tag. This PR introduces changes to the metadata field that contain the
originalName
origin.kubeconfigand
pathID`Changes include:
UI fix for App home view,
Origin
now displays correct Kubeconfig pathmetadata field now includes
cluster.metadata.origin.kubeconfig
andcluster.metadata.originalName
andcluster.metadata.pathID
OriginalName
now displayed in non-dynamic cluster settingsHow to test
Name
section