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

Merged
merged 4 commits into from
Nov 11, 2024

Conversation

idogada-akamai
Copy link
Contributor

@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

crobby
crobby previously requested changes Sep 26, 2024
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
Contributor 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
Contributor 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
Contributor Author

@crobby
Fixed the linting error

@idogada-akamai
Copy link
Contributor Author

@enrichman I re-used the context for the test

@crobby crobby dismissed their stale review November 7, 2024 15:07

updates made

@idogada-akamai
Copy link
Contributor Author

idogada-akamai commented Nov 11, 2024

@crobby @enrichman
What are our next steps to getting this pushed?

@crobby
Copy link
Contributor

crobby commented Nov 11, 2024

@crobby @enrichman What are our next steps to getting this pushed?

Hopefully, we can get reviewed and merged "soon". I'm not sure which Rancher release will receive this change yet.

@crobby crobby merged commit f5d2eaa into rancher:main Nov 11, 2024
1 check passed
@nickvth
Copy link

nickvth commented Nov 14, 2024

Releasing this change, would make me happy 😃 So any ideas when?

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.

5 participants