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

[feature] add command get k3stoken to retrieve cluster token #257

Merged
merged 7 commits into from
Jun 4, 2020

Conversation

RouxAntoine
Copy link
Contributor

This pull request

🔧 Update Makefile and .gitignore
♻️ Some side effect refactoring
✨ Add get k3stoken feature

More information and discussion here : #211

@RouxAntoine
Copy link
Contributor Author

RouxAntoine commented Jun 1, 2020

I'm fixing CI and the tests :)

Copy link
Member

@iwilltry42 iwilltry42 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, it looks great! :)
However, I'm unsure if it should really be a separate command or if it should rather be part of k3d get cluster, e.g. as a --token flag or via extending the whole get cluster command with JSON/YAML/template output, where the token can easily be retrieved from. What do you think?

cmd/root.go Outdated Show resolved Hide resolved
pkg/cluster/cluster.go Outdated Show resolved Hide resolved
pkg/cluster/cluster.go Outdated Show resolved Hide resolved
pkg/cluster/cluster.go Outdated Show resolved Hide resolved
@RouxAntoine
Copy link
Contributor Author

RouxAntoine commented Jun 2, 2020

Yes good idea i will change it 👍
Add --token to get cluster add token column to output result.

@RouxAntoine RouxAntoine force-pushed the feature/add-get-token-v2 branch from 18811fe to 1c0028d Compare June 3, 2020 00:36
@iwilltry42 iwilltry42 self-requested a review June 3, 2020 12:04
@iwilltry42
Copy link
Member

This already looks pretty good :)
Only I'd still prefer get cluster --token (extended output) over get k3stoken 👍

@RouxAntoine
Copy link
Contributor Author

Yes i will do change for --token.

Question about another topic do you know why CI complain about that : cannot use &(k3d.Cluster literal) (value of type *types.Cluster) as context.Context value in argument to cluster.GetCluster: missing method Deadline for method cluster.GetCluster and cluster.GetClusters ?

Maybe related to this change :

	- Secret             string             `yaml:"cluster_secret" json:"clusterSecret,omitempty"`
	+ Token              string             `yaml:"cluster_token" json:"clusterToken,omitempty"`

@iwilltry42
Copy link
Member

@RouxAntoine the error is due to recent changes in master, introducing context.Context everywhere.
I created a PR to your repo which merges master into your feature branch and fixes the aforementioned issues 👍 (I don't know how merging this stuff will work)

@RouxAntoine
Copy link
Contributor Author

Thanks 👌

RouxAntoine and others added 7 commits June 4, 2020 00:43
Add go compiler flag to makefile
Add makefile target with debug compiler flag
Exclude Idea project file
Print usage When no k3d verb was specify
Replace fake Cluster object create into getKubeConfig by realy complete object obtain thanks to cluster.GetCluster() function
Use constant variable for node Label name use into populateClusterFieldsFromLabels() function, in forecast to rename label `k3d.cluster.secret` to `k3d.cluster.token`
Add new command verb `k3stoken` to `k3d get`
Populate cluster secret field
Add test for `k3d get k3stoken` command
Externalize validable flag `--all`, in anticipation to reuse into `getKubeConfig.go`
Use `log.Fatalln(err)` instead of `log.Errorln(err)` + `os.Exit(1)`
Use Label prefix instead of LabelName suffix
Rename all secret occurence with token
@RouxAntoine RouxAntoine force-pushed the feature/add-get-token-v2 branch 2 times, most recently from 40d47b1 to 0862f11 Compare June 3, 2020 23:16
@RouxAntoine
Copy link
Contributor Author

RouxAntoine commented Jun 3, 2020

That's it, open for second review :) with this new --token version

Copy link
Member

@iwilltry42 iwilltry42 left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -37,96 +37,90 @@ import (
"github.com/liggitt/tabwriter"
)

// TODO : deal with --all flag to manage differentiate started cluster and stopped cluster like `docker ps` and `docker ps -a`
Copy link
Member

Choose a reason for hiding this comment

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

Out of scope for this PR, but still worth mentioning: during development of k3d v1.x, we had this differentiation and the community didn't like it, so we made --all the default.
I think we should rather stick to kubectl-similar style and show all clusters, when no flag is set and only filter them by setting new flags.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes i think i will update getkubeconfig in another PR to remove --all flag. I am not sure to understand, k3d style should show only started cluster without --all or not ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made proposal for this change here #268.

@iwilltry42
Copy link
Member

This will certainly be a breaking change, but I really like it 👍
Thanks a lot for your contribution :)

@iwilltry42 iwilltry42 merged commit 92c1157 into k3d-io:master Jun 4, 2020
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