-
Notifications
You must be signed in to change notification settings - Fork 169
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
backend: refactor listing all contexts and error #2326
Conversation
Backend Code coverage changed from 60.1% to 61.6%. Change: 1.5% 😃. |
9a586c6
to
0ee9a5a
Compare
Backend Code coverage changed from 60.1% to 61.6%. Change: 1.5% 😃. |
0ee9a5a
to
f39dbd8
Compare
Backend Code coverage changed from 60.1% to 61.9%. Change: 1.8% 😃. |
f39dbd8
to
8daa9d0
Compare
Backend Code coverage changed from 60.1% to 61.7%. Change: 1.6% 😃. |
8daa9d0
to
119a645
Compare
Backend Code coverage changed from 60.2% to 61.7%. Change: 1.5% 😃. |
119a645
to
a07ce59
Compare
Backend Code coverage changed from 60.2% to 61.6%. Change: 1.4% 😃. |
a07ce59
to
a404b72
Compare
Backend Code coverage changed from 60.2% to 61.6%. Change: 1.4% 😃. |
a404b72
to
5d0ceee
Compare
Backend Code coverage changed from 60.1% to 61.8%. Change: 1.7% 😃. Coverage report
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 thanks for this
I think we can merge this because it fixes some issues, and generally improves things. However, in the future I think we need to look into using context Name or not. It's not really unique or even required to be there strictly (however for 99% of users it probably is there). We need something which is unique across clusters, and is stable after a restart. Currently much of the frontend assumes this value is non-empty and unique - which isn't true strictly. Leaving my comments here for future work...I'm not sure about using "" here. Why was that chosen over an empty string "" or nil? kubectl just gives nothing if a context doesn't have a name.
kubectl can work without a name, because it uses the current context. I also wonder about the uniqueness of the name if there are multiple ones with out a name? Ultimately we'd want to ask the user if they can set a name? Additionally, what about using the other two fields? User name and cluster name? Maybe we can also handle this case outside of this PR. The errors and context could be returned as a parallel array, so that at least in our code the error can be matched up with the context no matter the name. |
This removes the dependacy of manually parsing of k8s resources. It was a problem as we had to be in sync with k8s API changes, which is not a good way. This change removes that and it splits kubeconfig into seperate contexts and validates them. Fixes: #2347 Co-Authored-By: Santhosh Nagaraj <sannagaraj@microsoft.com> Signed-off-by: Kautilya Tripathi <ktripathi@microsoft.com>
5d0ceee
to
f01f0f6
Compare
Backend Code coverage changed from 60.1% to 61.8%. Change: 1.7% 😃. Coverage report
|
This removes the dependacy of manually parsing of k8s resources. It was a problem as we had to be in sync with k8s API changes, which is not a good way. This change removes that and it splits kubeconfig into seperate contexts and validates them.
Testing
cd backend && go run ./cmd -dev
cd frontend && npm start
Reference
#2289 (comment)