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

Allow user to select Kubeconfig from filesystem #740

Merged
merged 17 commits into from Aug 28, 2020

Conversation

ixrock
Copy link
Member

@ixrock ixrock commented Aug 24, 2020

image

image

close #738

Signed-off-by: Roman <ixrock@gmail.com>
@ixrock ixrock requested a review from a team August 24, 2020 16:26
@jakolehm jakolehm added area/ui enhancement New feature or request labels Aug 24, 2020
@jakolehm jakolehm added this to the 3.6.0 milestone Aug 24, 2020
render() {
const { onSelectFiles, readAsText, ...props } = this.props;
return (
<input
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we somehow set a directory here? It would make sense to open a file dialog to a path where default kubeconfig is located (thanks @matti for this tip).

Copy link
Member Author

Choose a reason for hiding this comment

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

Have to check if it's possible with standard <input type="file"> system dialog to customize it somehow.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not normally allowed because of browser security issues. Electron could have an extension to this behaviour because it also allows to read local files (which browsers don't allow).

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@aleksfront aleksfront left a comment

Choose a reason for hiding this comment

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

Also, is it possible to show hidden files and folders in upload dialog? It's impossible to go to some paths because of that.

src/renderer/components/+add-cluster/add-cluster.scss Outdated Show resolved Hide resolved
Copy link
Contributor

@jakolehm jakolehm left a comment

Choose a reason for hiding this comment

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

LGTM. We probably should open an issue about using electron's showOpenDialog (instead of input).

@aleksfront
Copy link
Contributor

An idea (out of the scope): should we also allow to drop kubeconfig files from filesystem right into this "input"?

@jakolehm
Copy link
Contributor

Also, is it possible to show hidden files and folders in upload dialog? It's impossible to go to some paths because of that.

That's why we should open the default kubeconfig location by default (because it's a hidden folder and not easily accessible on MacOS).

@jakolehm
Copy link
Contributor

An idea (out of the scope): should we also allow to drop kubeconfig files from filesystem right into this "input"?

Good idea!

@jakolehm
Copy link
Contributor

Also, is it possible to show hidden files and folders in upload dialog? It's impossible to go to some paths because of that.

That's why we should open the default kubeconfig location by default (because it's a hidden folder and not easily accessible on MacOS).

Actually it's possible to show hidden files/folders with https://www.electronjs.org/docs/api/dialog#dialogshowopendialogbrowserwindow-options . Anyway, we should still open a dialog to a folder where current kubeconfig is.

@ixrock
Copy link
Member Author

ixrock commented Aug 25, 2020

BTW it's possible to show hidden files in finder in MacOS with Cmd+Shift+.

ixrock and others added 4 commits August 25, 2020 10:56
Co-authored-by: Jari Kolehmainen <jari.kolehmainen@gmail.com>
Co-authored-by: Alex Andreev <alex.andreev.email@gmail.com>
Signed-off-by: Roman <ixrock@gmail.com>
@ixrock
Copy link
Member Author

ixrock commented Aug 25, 2020

New select files system dialog:
Screenshot 2020-08-25 at 13 50 16

Drag-n-drop custom kube-config:
Screenshot 2020-08-25 at 14 00 31

@jakolehm
Copy link
Contributor

@ixrock awesome! Could you make DCO bot happy so that we can merge this asap?

@miskun
Copy link
Member

miskun commented Aug 25, 2020

UI needs improvement. The feature itself is good.

@jakolehm
Copy link
Contributor

UI needs improvement. The feature itself is good.

@miskun let's create a separate issue about this.

Signed-off-by: Roman <ixrock@gmail.com>
… kube-config file

Signed-off-by: Roman <ixrock@gmail.com>
…auto-replace "~" => os.homedir()

Signed-off-by: Roman <ixrock@gmail.com>
@ixrock
Copy link
Member Author

ixrock commented Aug 25, 2020

Latest design. Wdyt?

Customized kube-config file view:
Icons at the right: Reset and Browse
Screenshot 2020-08-25 at 23 18 28

Custom config tab:
Should we customize height/position of the editor somehow?
Screenshot 2020-08-25 at 23 19 01

Dropping kube-config file in case of invalid file
image

@ixrock
Copy link
Member Author

ixrock commented Aug 25, 2020

One more question: in case of using second tab with custom kube-config text: should we still use "Select context" form-control or hide it and instead consume all clusters and contexts from pasted config?

@aleksfront
Copy link
Contributor

Can you replace kube-config in tab and in Pro-tip to just kubeconfig. K8S docs are using latest notation.

@aleksfront
Copy link
Contributor

One more question: in case of using second tab with custom kube-config text: should we still use "Select context" form-control or hide it and instead consume all clusters and contexts from pasted config?

My suggestion is:
Select kubeconfig file tab - leave as is but remove Custom option from context selector.
Paste as text tab - remove context selector completely and create all pasted clusters.

@matti
Copy link

matti commented Aug 26, 2020

umm, if the config has a lot of contexts it will take about an hour with the current "how to remove cluster from lens" - it is a LOT of clicking. And I think doing that kind of thing "add all clusters" is horribly wrong decission anyway. The user is doing "add cluster" - not "add clusterS"

@jakolehm
Copy link
Contributor

umm, if the config has a lot of contexts it will take about an hour with the current "how to remove cluster from lens" - it is a LOT of clicking. And I think doing that kind of thing "add all clusters" is horribly wrong decission anyway. The user is doing "add cluster" - not "add clusterS"

Good point, thanks @matti !

Signed-off-by: Roman <ixrock@gmail.com>
Signed-off-by: Roman <ixrock@gmail.com>
@ixrock
Copy link
Member Author

ixrock commented Aug 26, 2020

Initial view
Screenshot 2020-08-26 at 20 17 00

Adding clusters from non-default kubeconfig file
Screenshot 2020-08-26 at 20 17 36
Screenshot 2020-08-26 at 20 20 01

Adding clusters from pasted kubeconfig
Screenshot 2020-08-26 at 20 20 50

Signed-off-by: Roman <ixrock@gmail.com>
@nevalla
Copy link
Contributor

nevalla commented Aug 27, 2020

I think it's now pre-selects all contexts found in kubeconfig. That might be fine if there's only one context but if there are hundreds of contexts it's not good idea.

@nevalla
Copy link
Contributor

nevalla commented Aug 27, 2020

When adding clusters from kubeconfig file, we don't want to save kubeconfig under app resources but we want to keep the reference to the original kubeconfig file. If pasting kubeconfig, then we store kubeconfig like we do it already.

@nevalla
Copy link
Contributor

nevalla commented Aug 27, 2020

Please could you run git rebase HEAD~11 --signoff to make DCO happy.

@nevalla
Copy link
Contributor

nevalla commented Aug 27, 2020

When adding clusters from kubeconfig file, we don't want to save kubeconfig under app resources but we want to keep the reference to the original kubeconfig file. If pasting kubeconfig, then we store kubeconfig like we do it already.

That said, we need to unlink kubeconfig file on cluster remove only if kubeconfig file is stored in app resources.

@ixrock
Copy link
Member Author

ixrock commented Aug 27, 2020

When adding clusters from kubeconfig file, we don't want to save kubeconfig under app resources but we want to keep the reference to the original kubeconfig file. If pasting kubeconfig, then we store kubeconfig like we do it already.

What happens if file is damaged or added context will be removed from original kubeconfig file at some point?

- remove only custom embed kubeconfig files
- keep link to original kubeconfig file when added via file
- remove auto-select all contexts from kubeconfig source

Signed-off-by: Roman <ixrock@gmail.com>
Signed-off-by: Roman <ixrock@gmail.com>
…stom_config

# Conflicts:
#	src/common/cluster-store.ts
#	src/migrations/cluster-store/3.6.0-beta.1.ts
Signed-off-by: Roman <ixrock@gmail.com>
@ixrock
Copy link
Member Author

ixrock commented Aug 27, 2020

@nevalla ready to merge, please check.
FYI: auto-selecting contexts enabled when up to 10 contexts available from kubeconfig source for now.

@@ -5,7 +5,7 @@ import { defineGlobal } from "./utils/defineGlobal";

export const isMac = process.platform === "darwin"
export const isWindows = process.platform === "win32"
export const isDebugging = process.env.DEBUG === "true";
export const isDebugging = process.env.DEBUG === "lens";
Copy link
Contributor

Choose a reason for hiding this comment

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

please revert this and let's focus on this in separate PR

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Signed-off-by: Roman <ixrock@gmail.com>
@nevalla
Copy link
Contributor

nevalla commented Aug 27, 2020

FYI: auto-selecting contexts enabled when up to 10 contexts available from kubeconfig source for now.

I'm not sure about this. I have one context in default kubeconfig that I don't ever want to add to Lens. Now it's always auto-selected when adding new clusters. @jakolehm WDYT?

@ixrock
Copy link
Member Author

ixrock commented Aug 27, 2020

FYI: auto-selecting contexts enabled when up to 10 contexts available from kubeconfig source for now.

I'm not sure about this. I have one context in default kubeconfig that I don't ever want to add to Lens. Now it's always auto-selected when adding new clusters. @jakolehm WDYT?

Well you can de-select manually. But for most cases auto-selection is good IMO.

@jakolehm
Copy link
Contributor

FYI: auto-selecting contexts enabled when up to 10 contexts available from kubeconfig source for now.

I'm not sure about this. I have one context in default kubeconfig that I don't ever want to add to Lens. Now it's always auto-selected when adding new clusters. @jakolehm WDYT?

IMHO auto-selection is not a good idea. I have had times when I have huge number of clusters in kubeconfig and normally I always just want to get one cluster from there.

Signed-off-by: Roman <ixrock@gmail.com>
Copy link
Contributor

@nevalla nevalla left a comment

Choose a reason for hiding this comment

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

LGTM

@nevalla nevalla merged commit b8994a4 into master Aug 28, 2020
@nevalla nevalla deleted the add_cluster_from_custom_config branch August 28, 2020 05:43
This was referenced Aug 28, 2020
@nevalla nevalla mentioned this pull request Sep 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ui enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow user to select Kubeconfig from filesystem
6 participants