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
| } | ||
|
|
||
| // this handles the clusters that have been renamed before the headlamp_info was introduced with original name | ||
| if headlampInfo != nil { |
There was a problem hiding this comment.
Again, this is not removed. Can you please remove it?
| @@ -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 |
| @@ -1025,3 +1028,58 @@ func TestHandleClusterHelm(t *testing.T) { | |||
| }) | |||
| } | |||
| } | |||
|
|
|||
| // test for handleHeadlampInfo, currently checks for headlamp_info field and headlamp_info.originalName field. | |||
| func (c *HeadlampConfig) TestHandleHeadlampInfo(t *testing.T) { | |||
There was a problem hiding this comment.
This function should call the existing function to check the functionality. This is the reason the test coverage does not increase. Can you please rewrite this? Thanks
There was a problem hiding this comment.
this test requires a bit of set up for it to work, mainly creating a temp mock env and then creating mock data within that env
I then convert the mock data from a api.Config structure into a kubeconfig.Context structure since that what handleHeadlampInfo test requires
I think this is necessary due to the placement/use of handleHeadlampInfo being within the getClusters function which I have mocked its functionality here (see the code leading up to handleHeadlampInfo at the top of getClusters)
I call the handleHeadlampInfo function, have it change the mock data, then check if the mock data has the headlamp_info fields and if the originalName is populated within headlamp_info
I will tag you where it's called
There was a problem hiding this comment.
The test function declaration is not correct. In Go, test functions should not be methods on a type. They should be standalone functions in the test file.
Suggested change
| func (c *HeadlampConfig) TestHandleHeadlampInfo(t *testing.T) { | |
| func TestHandleHeadlampInfo(t *testing.T) { | |
| // Create config instance for testing | |
| config := &HeadlampConfig{ | |
| // initialize necessary fields for testing | |
| } | |
| // Rest of the test code using 'config' instead of 'c' | |
| ... | |
| } |
There was a problem hiding this comment.
Currently it is lacking 3 main things
- Test Cases Organization: The test lacks clear separation of test cases. It should test different scenarios:
tests := []struct {
name string
configFile string
expectedError bool
validateFunc func(*testing.T, *kubeconfig.Context)
}{
{
name: "new headlamp_info creation",
configFile: "kubeconfig_headlampinfo_empty",
validateFunc: func(t *testing.T, ctx *kubeconfig.Context) {
info := ctx.KubeContext.Extensions["headlamp_info"]
require.NotNil(t, info, "headlamp_info should be created")
customObj, err := ParseHeadlampInfo(info)
require.NoError(t, err)
assert.Equal(t, ctx.Name, customObj.OriginalName)
},
},
{
name: "update legacy headlamp_info",
configFile: "kubeconfig_headlampinfo_legacy",
validateFunc: func(t *testing.T, ctx *kubeconfig.Context) {
info := ctx.KubeContext.Extensions["headlamp_info"]
require.NotNil(t, info, "headlamp_info should exist")
customObj, err := ParseHeadlampInfo(info)
require.NoError(t, err)
assert.NotEmpty(t, customObj.OriginalName, "originalName should be set")
assert.NotEmpty(t, customObj.CustomName, "customName should be preserved")
},
},
}- Type Safety Improvements: Instead of using type assertion for
map[string]interface{}, use the CustomObject type: - Helper Functions: Extract common setup code. Just sample name are below
func TestHandleHeadlampInfo(t *testing.T) {
// IMPROVEMENT: Consider creating a newTestConfig() helper function instead
// of inline configuration for better reusability across tests
config := &HeadlampConfig{
kubeConfigStore: kubeconfig.NewContextStore(),
}
// IMPROVEMENT: Add more fields to the test struct:
// - description string: for detailed test documentation
// - setupFunc: for test-specific setup if needed
tests := []struct {
name string
configFile string
expectedError bool
validateFunc func(*testing.T, *kubeconfig.Context)
}{
// MISSING TEST CASES:
// 1. Empty headlamp_info
// 2. Existing valid headlamp_info
// 3. Invalid/malformed headlamp_info
// 4. Missing required fields
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
// GOOD: Setup is separated into helper functions
// SUGGESTION: Add cleanup in defer if needed
mockConfigFile, config := setupTestKubeconfig(t, tt.configFile)
contexts := contextFromConfig(config)
// IMPROVEMENT: Consider combining Execute and Validate loops
// to reduce iterations over contexts
for _, ctx := range contexts {
// MAIN TEST: This is where handleHeadlampInfo is called
// It tests:
// 1. Creation of new headlamp_info for empty extensions
// 2. Update of existing headlamp_info for legacy contexts
// 3. Error handling for invalid configurations
err := config.handleHeadlampInfo(ctx)
// ERROR CHECK: Verify expected error behavior
// If expectedError is true, we should get an error
// If false, we should not get an error
if tt.expectedError {
require.Error(t, err)
return
}
require.NoError(t, err)
// VALIDATION: Run the test-case specific validation
// This checks the actual content and structure
// of the headlamp_info extension
tt.validateFunc(t, ctx)
}
// IMPROVEMENT: Add timeout context for long-running tests
// PERSISTENCE CHECK: Verify changes were actually saved
// SUGGESTION: Add more specific checks for the content
// of headlamp_info, not just its presence
persistedConfig, err := clientcmd.LoadFromFile(mockConfigFile)
require.NoError(t, err)
// FINAL VALIDATION: Ensure changes persisted to disk
// Verify each context has the expected headlamp_info
for name, ctx := range persistedConfig.Contexts {
info := ctx.Extensions["headlamp_info"]
require.NotNil(t, info, "headlamp_info should be persisted for context %s", name)
}
})
}
}
vyncent-t
commented
Jan 24, 2025
| // use the kubeContexts slice for further processing | ||
| for _, ctx := range kubeContexts { | ||
| // Call handleHeadlampInfo | ||
| err = c.handleHeadlampInfo(ctx) |
There was a problem hiding this comment.
@knrt10 I call the func here but for it to be ran I first have to create a mock env and then take the test data and turn it into kubeconfig.Context since I can't use config directly (its type is api.Config and the handleHeadlampInfo takes kubeconfig.Context)
In the two for loops under it I check if the headlamp_info field exists on all the contexts and I also check if all the originalName field exists within headlamp_info in each context with another loop
…nfo on getClusters calls Signed-off-by: Vincent T <[email protected]>
…eadlamp_info changes 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
8724036 to
5b62830
Compare
|
Backend Code coverage changed from 64.7% to 64.9%. Change: .2% 😃. Coverage report |
vyncent-t
changed the title
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