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

context can be 1 or bigger #636

Merged
merged 6 commits into from
Mar 15, 2023
Merged

context can be 1 or bigger #636

merged 6 commits into from
Mar 15, 2023

Conversation

AdamRussak
Copy link
Contributor

added >= to if len(kc.config.Contexts) >= 1, as it could be a user has just 1 context in current file

added >= to  if len(kc.config.Contexts) >= 1, as it could be a user has just 1 context in current file
@codecov
Copy link

codecov bot commented Feb 28, 2023

Codecov Report

Merging #636 (2235b51) into master (6f37452) will not change coverage.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##           master     #636   +/-   ##
=======================================
  Coverage   13.48%   13.48%           
=======================================
  Files          19       19           
  Lines        1891     1891           
=======================================
  Hits          255      255           
  Misses       1615     1615           
  Partials       21       21           
Flag Coverage Δ
unittests 13.48% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cmd/add.go 38.51% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@sunny0826
Copy link
Owner

@AdamRussak The kc.config.Contexts is only used to rename Contexts when there are multiple contexts in them (rule: filename + Hash of context) and does not affect multiple contexts being added to kubeconfig.

You can see the exact effect in the output of the e2e test.

@AdamRussak
Copy link
Contributor Author

The fix addresses a scenario of a single context in the file.
It shouldn't be renamed, but it will.

I will go over the test and try to show it.

P.s
G8t project!

@sunny0826
Copy link
Owner

To cope with the problem of duplicate context names, the context is renamed each time it is added.

This is indeed a problem and is discussed in #602, but I haven't had time to address it so far.

@AdamRussak
Copy link
Contributor Author

i overcame it in my implementation.
Will look for the time to contribute to your project.
But will need assistance with the testing part.

@sunny0826
Copy link
Owner

I will help you

@AdamRussak
Copy link
Contributor Author

@sunny0826 please take a look (only) at the latest test run.
*here are the older once if you want to see: https://github.com/AdamRussak/kubecm/actions/workflows/e2e-test.yaml

some explanation:
in cmd/add.go line 119:

if len(kc.config.Contexts) > 1 {
			newName = fmt.Sprintf("%s-%s", kc.fileName, HashSufString(name))
		} else {
			newName = name
		}

I replaced the newName from kc.filename with just name.
the logic behind it is that it will keep the current context name of the cluster AS IS and will validate duplication in the following step.
I updated the context name for the switch test in the E2E pipeline to reflect that name as it shows in the output:
https://github.com/AdamRussak/kubecm/actions/runs/4419494196/jobs/7747985147#step:10:109

this should also resolve issue #602

Copy link
Owner

@sunny0826 sunny0826 left a comment

Choose a reason for hiding this comment

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

It seems that this PR does not trigger the e2e test

@sunny0826
Copy link
Owner

It seems that this PR does not trigger the e2e test

Oh, it worked

@AdamRussak AdamRussak requested a review from sunny0826 March 15, 2023 07:08
@AdamRussak
Copy link
Contributor Author

@sunny0826 i see you requested some changes, but I cant see what changes you requested.

@sunny0826
Copy link
Owner

@sunny0826 i see you requested some changes, but I cant see what changes you requested.

Sorry, I'm just waiting for the e2e test to finish running.

@sunny0826
Copy link
Owner

LGTM
Thanks for your contribution!

@sunny0826 sunny0826 merged commit ef12336 into sunny0826:master Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants