-
Notifications
You must be signed in to change notification settings - Fork 453
Allow using K0S_TOKEN environ as source of Join token #6766
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
base: main
Are you sure you want to change the base?
Conversation
09f0c7d to
b8a7fd6
Compare
twz123
left a comment
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.
Can you add some sentences / an example about the environment variable to the documentation, e.g. to k0s-multi-node.md?
pkg/token/tokendata.go
Outdated
| } | ||
|
|
||
| if tokenSources > 1 { | ||
| return "", fmt.Errorf("you can only pass one token source: either as a CLI argument, via '--token-file [path]', or via the %s environment variable", constant.EnvVarToken) |
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.
I wouldn't make the presence of the environment variable a hard error. It should rather be a fallback if the token hasn't been passed as an arg or via the file flag.
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.
It would be unclear in that case what token was used, do you want to add ambiguous behavior and solve that by extending documentation and telling people why passing token via env won't take precedence over a flag?
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.
I guess it's standard behavior, isn't it? We have a flag (although it's an optional arg in this case), and a related environment variable as a fallback. If the flag (arg) is present, it'll use that, if the flag is missing, it'll fall back to the env.
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.
no, it is not standard, what you are describing is optional reconfiguration with environ, PR is after having a clear way of setting token only with one option and if that fails report error instead of trying to run k0s cluster and fail expectation
| if len(args) > 0 { | ||
| c.TokenArg = args[0] | ||
| } | ||
| if c.TokenArg != "" && c.TokenFile != "" { |
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.
I'd prefer to leave this check in place (or refactor it out and reuse it here via a function, whichever makes more sense.) We should verify the command line's correctness before attempting to do anything.
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.
hopefully handled by internal.CheckSingleTokenSource
b8a7fd6 to
6776625
Compare
Signed-off-by: s3rj1k <[email protected]>
Signed-off-by: s3rj1k <[email protected]>
Signed-off-by: s3rj1k <[email protected]>
3c08fee to
d266af6
Compare
Implements ability to pass Join token via K0S_TOKEN environ.
This is alternative to #6755