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

Avoid overwriting entire KubeCredentials block #397

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

idogada-akamai
Copy link

@idogada-akamai idogada-akamai commented Sep 22, 2024

As described in rancher/rancher#46997

Whenever we use rancher token to get a token of cluster we're currently on, instead of just verifying the token expiration date and exiting, the CLI, asks for a re-login and clears the KubeCredentials block.

This PR aims to fix that issue

Copy link
Contributor

@crobby crobby left a comment

Choose a reason for hiding this comment

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

Looks like a fine change.
Can you add a small unit test to cover this?

@idogada-akamai
Copy link
Author

Looks like a fine change. Can you add a small unit test to cover this?
@crobby Can you guide me on how to do that?
I found some stuff around the project, but it's not clear to me

@crobby
Copy link
Contributor

crobby commented Oct 7, 2024

Looks like a fine change. Can you add a small unit test to cover this?
@crobby Can you guide me on how to do that?
I found some stuff around the project, but it's not clear to me

The new test would go in kubectl_token_test.go. We would only need to test the cacheCredential function.
It might be a little bit awkward since I think as it's written right now, you may have to have your test write a config file that would serve as a possible existing configuration so that it can be loaded and rewritten by the function. The test would then need to check the contents of that file to make sure it contains/doesn't contain the expected entries.

@bigkevmcd
Copy link

I've written a test for this fix here 78c6a7c feel free to copy the test and update the PR, or we can merge it.

@idogada-akamai
Copy link
Author

I've written a test for this fix here 78c6a7c feel free to copy the test and update the PR, or we can merge it.

@bigkevmcd Thank you! I have added the test.

@crobby Can you please review the change and let us know if any further change is required?

@bigkevmcd bigkevmcd requested a review from crobby October 10, 2024 14:37
@crobby
Copy link
Contributor

crobby commented Oct 14, 2024

I've written a test for this fix here 78c6a7c feel free to copy the test and update the PR, or we can merge it.

@bigkevmcd Thank you! I have added the test.

@crobby Can you please review the change and let us know if any further change is required?

Will do.
Thank you.

It looks like the linter has identified a few issues. You should be able to see the logs and adjust based on that.

@idogada-akamai
Copy link
Author

@crobby
Fixed the linting error

@idogada-akamai
Copy link
Author

@enrichman I re-used the context for the test

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.

4 participants