-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
fix kind get clusters
via nerdctl
#3814
fix kind get clusters
via nerdctl
#3814
Conversation
Now raising errors when using nerdctl for the provider ```console > ./kind --version kind version 0.26.0-alpha > nerdctl --version nerdctl version 2.0.1 > ./kind get clusters ERROR: failed to list clusters: command "nerdctl ps -a --filter label=io.x-k8s.kind.cluster --format '{{index .Labels "io.x-k8s.kind.cluster"}}'" failed with error: exit status 1 Command Output: time="2024-12-09T18:01:03+09:00" level=fatal msg="template: :1:2: executing \"\" at <index .Labels \"io.x-k8s.kind.cluster\">: error calling index: cannot index slice/array with type string" ``` nerdctl fixed the `.Label` behavior in v1.7.0. containerd/nerdctl@2af4cef However `index .Labels` syntax is not yet supported at least in v2.0.1. (The style is also used for podman provider, and it is available) This commit follows up #3429 Signed-off-by: Kenichi Kamiya <kachick1@gmail.com>
The committers listed above are authorized under a signed CLA. |
Welcome @kachick! |
Hi @kachick. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
/ok-to-test @AkihiroSuda can you ping some nerdctl maintainers to check if this is correct? Nevermind, I see the PR is yours, let me assign this to you /assign @AkihiroSuda |
@@ -125,7 +125,7 @@ func (p *provider) ListClusters() ([]string, error) { | |||
// filter for nodes with the cluster label | |||
"--filter", "label="+clusterLabelKey, | |||
// format to include the cluster name | |||
"--format", fmt.Sprintf(`{{index .Labels "%s"}}`, clusterLabelKey), | |||
"--format", fmt.Sprintf(`{{.Label "%s"}}`, clusterLabelKey), |
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.
Do we know if this ever worked? Do we need to version gate it? Or was it just copy-pasted from the docker provider and never worked?
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.
Probably this was just copied without testing?
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.
This kind of difference should be mentioned in code comment lines
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 seems podman also already implemented this containers/podman#20958
It is using the old format
kind/pkg/cluster/internal/providers/podman/provider.go
Lines 110 to 111 in f3a8344
// format to include the cluster name | |
"--format", fmt.Sprintf(`{{index .Labels "%s"}}`, clusterLabelKey), |
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.
We should check the docker driver and probably re-align this.
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AkihiroSuda, aojea, kachick The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Thanks for reviewing and merging 🙏 |
Thanks all! |
Fixes #3813
nerdctl fixed the
.Label
behavior in v1.7.0.containerd/nerdctl#2619
However
index .Labels
syntax is not yet supported at least in v2.0.1.(The style is also used for podman provider, and it is available)
This PR aims to follow up #3429