drop shlex dependency #5943

koba1t · 2025-07-11T19:09:28Z

Remove dependency on shlex package by implementing functionality for shlex.Split()
ref: kubernetes/kubernetes#132593 (comment)

k8s-ci-robot · 2025-07-11T19:09:32Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

k8s-ci-robot · 2025-07-11T19:09:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: koba1t

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [koba1t]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2025-07-13T18:54:47Z

This PR has multiple commits, and the default merge method is: merge.
You can request commits to be squashed using the label: tide/merge-method-squash

Details

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.

stormqueen1990

Hi there, @koba1t! This change looks good to me in essence, I just have a suggestion regarding documentation and a question on the test cases.

stormqueen1990 · 2025-07-13T21:06:24Z

api/internal/plugins/execplugin/shlex.go

+		case (r == '\'' || r == '"') && quote == 0:
+			quote = r
+		case r == quote:
+			quote = 0
+		case r == '#' && quote == 0:


Could we use a constant for the indication that a quote was finished? It wasn't immediately clear to me why the value of quote here was 0 to mean "no quote open".

That's a good opinion.
I added a noQuote const value to check.
3866a30

stormqueen1990 · 2025-07-13T21:11:39Z

api/internal/plugins/execplugin/shlex_test.go

+			if tc.wantErr {
+				if err == nil {
+					t.Errorf("FAIL: Expected error but got none, Expected: %q\n", tc.expected)
+				}
+				return
+			}
+
+			if assert.NoError(t, err, "FAIL: Unexpected error %q for input %q", err, tc.input) {
+				// check if the result matches the expected output
+				assert.Equal(t, tc.expected, result,
+					"FAIL: Result mismatch,Input %q, Expected %q, Got: %q\n",
+					tc.input, tc.expected, result,
+				)
+			}


I'm wondering if we want require.Error/require.NoError in this loop instead of just assert. What do you think?

The difference between require and assert functions in whether execution continues when a test fails—in this test function, specifying either should produce identical behavior. This is because both assert variants cause the t.Run() function to terminate immediately upon failure.

https://pkg.go.dev/github.com/stretchr/testify/require

Makes sense. Let's keep as-is.

stormqueen1990

/lgtm

This MR contains the following updates: | Package | Update | Change | |---|---|---| | [kubernetes-sigs/kustomize](https://github.com/kubernetes-sigs/kustomize) | patch | `v5.7.0` -> `v5.7.1` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>kubernetes-sigs/kustomize (kubernetes-sigs/kustomize)</summary> ### [`v5.7.1`](https://github.com/kubernetes-sigs/kustomize/releases/tag/kustomize/v5.7.1) [Compare Source](kubernetes-sigs/kustomize@kustomize/v5.7.0...kustomize/v5.7.1) This release introduces code to replace the shlex library used for parsing arguments in the exec plugin. If any existing manifests become corrupted, please file an issue. discussion: https://github.com/kubernetes/kubernetes/pull/132593#discussion\_r2178116543 #### Dependencies [#5943](kubernetes-sigs/kustomize#5943): drop shlex dependency #### Chore [#5948](kubernetes-sigs/kustomize#5948): Update kyaml to v0.20.1 [#5949](kubernetes-sigs/kustomize#5949): Update cmd/config to v0.20.1 [#5950](kubernetes-sigs/kustomize#5950): Update api to v0.20.1 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 11, 2025

k8s-ci-robot requested a review from ncapps July 11, 2025 19:09

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 11, 2025

k8s-ci-robot requested a review from varshaprasad96 July 11, 2025 19:09

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 11, 2025

koba1t force-pushed the chore/drop_shlex_dependency branch 3 times, most recently from d2e62c5 to 95c954b Compare July 11, 2025 21:38

add testcases for shlexsplit

042a2cf

koba1t force-pushed the chore/drop_shlex_dependency branch from 95c954b to 042a2cf Compare July 11, 2025 22:14

add ShlexSplit() as an alternative to shlex.Split()

b612895

koba1t marked this pull request as ready for review July 13, 2025 18:54

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 13, 2025

k8s-ci-robot requested a review from stormqueen1990 July 13, 2025 18:55

remove shlex dependencies

5cb1b4e

stormqueen1990 reviewed Jul 13, 2025

View reviewed changes

introduce one const value that indicate to no quote in ShlexSplit()

3866a30

stormqueen1990 reviewed Jul 20, 2025

View reviewed changes

k8s-ci-robot assigned stormqueen1990 Jul 20, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 20, 2025

k8s-ci-robot merged commit 87f462a into kubernetes-sigs:master Jul 20, 2025
11 checks passed

soltysh mentioned this pull request Jul 22, 2025

Update kubectl kustomize to kustomize/v5.7.1 kubernetes/kubernetes#132593

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

drop shlex dependency #5943

drop shlex dependency #5943

Uh oh!

koba1t commented Jul 11, 2025

Uh oh!

k8s-ci-robot commented Jul 11, 2025

Uh oh!

k8s-ci-robot commented Jul 11, 2025

Uh oh!

k8s-ci-robot commented Jul 13, 2025

Uh oh!

stormqueen1990 left a comment

Uh oh!

stormqueen1990 Jul 13, 2025

Uh oh!

koba1t Jul 14, 2025

Uh oh!

stormqueen1990 Jul 13, 2025

Uh oh!

koba1t Jul 14, 2025

Uh oh!

stormqueen1990 Jul 20, 2025

Uh oh!

stormqueen1990 left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

drop shlex dependency #5943

drop shlex dependency #5943

Uh oh!

Conversation

koba1t commented Jul 11, 2025

Uh oh!

k8s-ci-robot commented Jul 11, 2025

Uh oh!

k8s-ci-robot commented Jul 11, 2025

Uh oh!

k8s-ci-robot commented Jul 13, 2025

Uh oh!

stormqueen1990 left a comment

Choose a reason for hiding this comment

Uh oh!

stormqueen1990 Jul 13, 2025

Choose a reason for hiding this comment

Uh oh!

koba1t Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

stormqueen1990 Jul 13, 2025

Choose a reason for hiding this comment

Uh oh!

koba1t Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

stormqueen1990 Jul 20, 2025

Choose a reason for hiding this comment

Uh oh!

stormqueen1990 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants