kubernetes-sigs / cri-tools

CLI and validation tools for Kubelet Container Runtime Interface (CRI) .
Apache License 2.0
1.69k stars 454 forks source link

Fix `crictl config --set` if the YAML defines entries multiple times #1661

Closed saschagrunert closed 4 weeks ago

saschagrunert commented 4 weeks ago

What type of PR is this?

/kind bug

What this PR does / why we need it:

Only the first entry would be written before applying this patch. This means that a config like this:

timeout: 5
timeout: 10
timeout: 20
runtime-endpoint: ""
image-endpoint: ""
debug: false
pull-image-on-create: false
disable-pull-on-run: false

Would return the right value (the last one):

> crictl --config ./crictl.yaml config --get timeout
20

But setting the config will only result in setting the first item:

> crictl --config ./crictl.yaml config --set timeout=30
> cat crictl.yaml
timeout: 30
timeout: 10
timeout: 20
runtime-endpoint: ""
image-endpoint: ""
debug: false
pull-image-on-create: false
disable-pull-on-run: false

And therefore also a wrong result of crictl config --get:

> crictl --config ./crictl.yaml config --get timeout
20

We now change that behavior and set all values without de-duplicating them:

> crictl --config ./crictl.yaml config --set timeout=30
> cat crictl.yaml
timeout: 30
timeout: 30
timeout: 30
runtime-endpoint: ""
image-endpoint: ""
debug: false
pull-image-on-create: false
disable-pull-on-run: false

e2e test cases around that scenario have been added as well.

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

Follow-up on https://github.com/kubernetes-sigs/cri-tools/pull/1659#discussion_r1817010069

Does this PR introduce a user-facing change?

Fixed `crictl config --set` if the configuration YAML defines entries multiple times.
k8s-ci-robot commented 4 weeks ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: saschagrunert, SergeyKanzhelev

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: - ~~[OWNERS](https://github.com/kubernetes-sigs/cri-tools/blob/master/OWNERS)~~ [saschagrunert] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment