fix the cluster info for headlamp_info to inlcude originalName #2747
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
|
progress: able now to have the cluster generate a headlamp_info field when it is created and gathered via the need to fix:
|
vyncent-t
force-pushed
the
keep-original-name-kubeconfig
branch
from
2136402 to
6248883
Compare
|
Backend Code coverage changed from 63.8% to 64.4%. Change: .6% 😃. Coverage report |
vyncent-t
force-pushed
the
keep-original-name-kubeconfig
branch
from
6248883 to
776c7c0
Compare
|
note: done
to do
|
|
let me know what you guys think for these changes before I move on to create some tests 😎 @knrt10 @yolossn contextThe main issue we are trying to solve is that there is currently no way to preserve the original name that a cluster has been created with, this has lead to issues with renaming / duplicate naming clusters with names that have been used already. Currently, on main if you create a cluster and rename it and then you console log the clusters that are being rendered at the home page, you will see that the "name" field populates the changed name while the kubeconfig still holds the old name The goal here is to make sure we create the |
dosubot
bot
added
the
size:L
|
Backend Code coverage changed from 63.8% to 64.2%. Change: .4% 😃. Coverage report |
vyncent-t
force-pushed
the
keep-original-name-kubeconfig
branch
from
776c7c0 to
0acb858
Compare
|
note: previous push fixes logic for clusters that were renamed already (with the headlamp_info already there) to update the headlamp_info to have their original name saved also |
@vyncent-t Thank you for the PR. I am not sure what this would solve. This was the intended change. Our approach initially was we do want to change the kubeconfig name of user because if user change the name in headlamp does not mean they want to change their kubeconfig. For example if they get their clusters using kubeclt they should still see the same name. This is the reason we do not change the name field in kubeconfig, rather than we edit the headlmap_info extension in the metadata. So when user renames the cluster only that thing is changed. Regarding clusters that use the same name. That I think should not be possible because we automatically throw an error if there are same context name. Also, if there are same name in kubeconfig that is automatically rejected by kubectl as well |
@knrt10 thanks for getting back to me 😎 allow me to clarify a few things and feel free to let me know if we should sync up to cover this This PR is a prerequisite for continuing the progress of the delete non dynamic clusters PR that you had reviewed for me earlier (along with two other issues that stem from this that I will explain later). In the delete non dynamic clusters PR's current state, the delete function needs a name in which to find and delete that cluster, however for clusters that have been renamed, that new customName is what is being being used in the app and thus when we go to delete that cluster on the app the customName is what is being passed into the delete function. The goal of this PR is to expand the This PR does not change the existing main data of a kubeconfig context, it only creates the I will include more information about what this PR does and how to test it in the description As for the clusters using the same name and renaming- I am also working on two issues that spawn from the current name handling for clusters. Currently we are able to create a cluster and rename it to the name of another cluster, causing the renamed cluster link to 'consume' the og cluster link. There is also the issue of creating a cluster with the og name of an older renamed cluster causing duplicate links. These were able to be done through renaming use and no errors stop this. I am also working on fixes for both of these but they too require this TLDR: I have been tasked with expanding the |
|
Thank you for explaining. It makes sense now. Will review it now. |
vyncent-t
force-pushed
the
keep-original-name-kubeconfig
branch
from
0acb858 to
24f7a6b
Compare
|
Backend Code coverage changed from 63.8% to 64.0%. Change: .2% 😃. Coverage report |
vyncent-t
force-pushed
the
keep-original-name-kubeconfig
branch
from
24f7a6b to
7c1a4aa
Compare
|
Backend Code coverage changed from 64.8% to 64.9%. Change: .1% 😃. Coverage report |
vyncent-t
force-pushed
the
keep-original-name-kubeconfig
branch
from
7c1a4aa to
8724036
Compare
|
@knrt10 made some changes from what you suggested and also added a test, I am using a mock env approach that seems to work when its ran at the end of the test, still new to tests so let me know what you think! I was also thinking of maybe using the format that you have for the TestRenameCluster if what I have here isn't the best way |
|
Backend Code coverage changed from 64.7% to 64.9%. Change: .2% 😃. Coverage report |
knrt10
requested changes
Jan 23, 2025
backend/pkg/kubeconfig/kubeconfig.go
Outdated
| @@ -56,9 +57,14 @@ type OidcConfig struct { | |||
|
|
|||
| // CustomObject represents the custom object that holds the HeadlampInfo regarding custom name. | |||
| type CustomObject struct { | |||
| // ??? | |||
There was a problem hiding this comment.
Oh! I forgot about these, I meant to ask if you could provide context here on what these params are/do?
There was a problem hiding this comment.
Suggested change
| // ??? | |
| type CustomObject struct { | |
| // TypeMeta describes the type of the object and its API schema version. | |
| // It should have "Kind" set to "HeadlampInfo" and "APIVersion" set to "v1". | |
| // +k8s:deepcopy-gen=false | |
| metav1.TypeMeta | |
| // ObjectMeta contains metadata about the custom object, such as name and labels. | |
| // This is used to store additional metadata about the headlamp_info extension. | |
| // +optional |
There was a problem hiding this comment.
Still not updated here.
vyncent-t
commented
Jan 24, 2025
vyncent-t
force-pushed
the
keep-original-name-kubeconfig
branch
from
8724036 to
5b62830
Compare
|
Backend Code coverage changed from 64.7% to 64.9%. Change: .2% 😃. Coverage report |
vyncent-t
force-pushed
the
keep-original-name-kubeconfig
branch
from
3e09a73 to
1d0edcf
Compare
|
Backend Code coverage changed from 64.8% to 65.5%. Change: .7% 😃. Coverage report |
dosubot
bot
added
size:XL
vyncent-t
force-pushed
the
keep-original-name-kubeconfig
branch
from
1d0edcf to
830a93e
Compare
|
Backend Code coverage changed from 65.0% to 65.5%. Change: .5% 😃. Coverage report |
vyncent-t
force-pushed
the
keep-original-name-kubeconfig
branch
from
830a93e to
9593a65
Compare
|
Backend Code coverage changed from 65.0% to 65.5%. Change: .5% 😃. Coverage report |
vyncent-t
force-pushed
the
keep-original-name-kubeconfig
branch
from
9593a65 to
87b0859
Compare
|
rebased to main
|
|
Backend Code coverage changed from 65.0% to 65.6%. Change: .6% 😃. Coverage report |
joaquimrocha
requested changes
Feb 14, 2025
backend/cmd/headlamp.go
Outdated
| ) | ||
|
|
||
| /* handler to keep the original name for a cluster.*/ | ||
| //nolint:lll |
| @@ -1136,6 +1142,72 @@ func (c *HeadlampConfig) getClusters() []Cluster { | |||
| return clusters | |||
| } | |||
|
|
|||
| // handleHeadlampInfo this function handles the clusters that need to have headlamp_info created or updated. | |||
There was a problem hiding this comment.
I don't understand exactly what this is doing from this comment. What does "handles the clusters" mean?
There was a problem hiding this comment.
I am a bit confused as to what exactly this patch is trying to achieve and why.
backend/pkg/kubeconfig/kubeconfig.go
Outdated
| @@ -56,9 +57,14 @@ type OidcConfig struct { | |||
|
|
|||
| // CustomObject represents the custom object that holds the HeadlampInfo regarding custom name. | |||
| type CustomObject struct { | |||
| // ??? | |||
There was a problem hiding this comment.
Still not updated here.
backend/pkg/kubeconfig/kubeconfig.go
Outdated
| metav1.TypeMeta | ||
| // ??? |
backend/pkg/kubeconfig/kubeconfig.go
Outdated
Comment on lines
59
to
66
| // ??? | ||
| // TypeMeta describes the type of the object and its API schema version. | ||
| // It should have "Kind" set to "HeadlampInfo" and "APIVersion" set to "v1". | ||
| // +k8s:deepcopy-gen=false | ||
| metav1.TypeMeta | ||
| // ??? | ||
| // ObjectMeta contains metadata about the custom object, such as name and labels. | ||
| // This is used to store additional metadata about the headlamp_info extension. | ||
| // +optional |
There was a problem hiding this comment.
Doesn't belong to this commit.
Comment on lines
242
to
244
| async function handleDynamicClusterName(cluster: string) { | ||
| findKubeconfigByClusterName(cluster); | ||
| } |
There was a problem hiding this comment.
Why do we need to have a handleDynamicClusterName function wrapping the new one? (and in general "handle*" is not a great name unless it's for something that's just a response, e.g. handleConfigRouteRequest).
frontend/src/stateless/index.ts
Outdated
| const contextToUpdate = matchingKubeconfig || matchingContext; | ||
| const extensions = contextToUpdate?.context.extensions || []; | ||
| let headlampExtension = extensions.find( | ||
| extension => extension.name === 'headlamp_info' |
There was a problem hiding this comment.
Why do stateless clusters need the headlamp_info extension? This extension's purpose was to avoid modifying the cluster name of clusters coming from "external" (to headlamp) kubeconfigs, and we were even talking the other day about whether we should even do this at all for those. But for anything that "belongs" or is defined in Headlamp, like stateless clusters, we should just change the actual context name when the user chooses to update it.
vyncent-t
force-pushed
the
keep-original-name-kubeconfig
branch
2 times, most recently
from
6011ae2 to
5157118
Compare
dosubot
bot
added
size:L
|
NOTE:
|
vyncent-t
force-pushed
the
keep-original-name-kubeconfig
branch
2 times, most recently
from
4efd536 to
5fd0090
Compare
|
vyncent-t
force-pushed
the
keep-original-name-kubeconfig
branch
3 times, most recently
from
ddbd526 to
e3158e1
Compare
…nfo on getClusters calls Signed-off-by: Vincent T <[email protected]>
Signed-off-by: Vincent T <[email protected]>
Signed-off-by: Vincent T <[email protected]>
vyncent-t
force-pushed
the
keep-original-name-kubeconfig
branch
from
e3158e1 to
e5ac4c5
Compare
|
Backend Code coverage changed from 65.0% to 65.2%. Change: .2% 😃. Coverage report |
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.
Fixes issue #2750
Description
This PR aims to expands the current implementation of the
headlamp_infoextension that is created to provide additional information about a cluster within the kubeconfig.Changes
headlamp_infoextension is now created at the home view of the app (it used to only be created if we rename a cluster)headlamp_infoextension now includes a field for storing the original name of a clusteroriginalNameheadlamp_infois now updated to include theoriginalNamefield and correct valheadlamp_infofield for future cluster renaming by introducing thecustomNamefield along withoriginalNameHow to test
headlamp_infofield andoriginalNamefield containing the clusters nameContext notes:
Non dynamic clusters