Conversation
cmoulliard
left a comment
There was a problem hiding this comment.
I like a lot this PR as it avoids for the user to hack the kind cfg file :-)
|
As we expose more and more kind config through CLI flags that directly corresponds to kind configurations and CLI flags, are we making it easier for people to use this tool? I think when we add flags, we should add a bit more value than exposing kind configuration options. Thinking about how the problem of credentials mounting is solved, if we expose kind config flag directly this is what that looks like: Do we want users to enter this every time? We can guess where registry configurations are located because docker and podman store them in To me, it makes more sense to provide a boolean flag indicating if the user wants to mount registry credentials and we do the rest. |
This is a fair point you reported here as yoou ask finally the question "What should be the boundary between what we propose as CLI flags/parameters vs passing such information using Why this is not the purpose of this ticket, it is nevertheless important that we keep in mind such an aspect and that we review what we did or will in a separate ticket to edict rules about that. WDYT ? @nabuskey |
As the ultimate goal is to mount the credentials of the user, then having a flag pointing to the config.json or auth.json file (docker vs podman) is probably more appropriate |
I am 100% in agreement here |
Yeah I was trying to take an approach somewhat between the two last night where we could have a |
dd7f82c to
055ea11
Compare
|
This is kinda hacky but seems reasonable to me. The flag can be used the following ways:
It is a little weird that this can take multiple |
nabuskey
left a comment
There was a problem hiding this comment.
The overall approach looks great to me. Thank you so much for this. Please take a look at my comments.
9a5605a to
c30b07f
Compare
|
I think my latest change addresses all of the comments. Let me know if it looks good, I also squashed the history and updated the PR to reflect the current state of this change. |
pkg/kind/cluster.go
Outdated
| } | ||
|
|
||
| if len(c.registryConfig) > 0 && registryConfig == "" { | ||
| setupLog.Info("--registry-config flag used but no registry config was found") |
There was a problem hiding this comment.
Do you think it's a good idea to fail here? We think the user's intention was to mount registry configuration, but it does not exist. If we do not fail, the cluster is created with configuration that does not match what the user wanted it to be. So it might be a better idea to fail early and ask the user for a valid configuration.
There was a problem hiding this comment.
Yeah I think that's a good idea, i threw a os.Exit(1) in that block
There was a problem hiding this comment.
I think it's better to return an error and let the error cascade out to cobra to have an unified experience for users. Right now all fatal errors go out like that.
There was a problem hiding this comment.
Yep that makes way more sense, pushed that change now
c30b07f to
c2e56a3
Compare
The new registry-config option when used alone looks up the default podman and docker registry config files and mounts them in the kind container if they exist. Optionally a user can pass a specific file to the flag to specify a custom path to mount as the registry config Signed-off-by: Caleb Boylan <calebboylan@gmail.com>
c2e56a3 to
32cc213
Compare
|
/e2e |
Signed-off-by: Caleb Boylan <calebboylan@gmail.com>
The new registry-config option when used alone looks up the default
podman and docker registry config files and mounts them in the kind
container if they exist. Optionally a user can pass a specific file to
the flag to specify a custom path to mount as the registry config.