Skip to content

🐛 Fix marker parser misinterpreting number-ish strings as numeric literals#1366

Open
sivchari wants to merge 2 commits into
kubernetes-sigs:mainfrom
sivchari:fix/marker-parser-numeric-literal-error
Open

🐛 Fix marker parser misinterpreting number-ish strings as numeric literals#1366
sivchari wants to merge 2 commits into
kubernetes-sigs:mainfrom
sivchari:fix/marker-parser-numeric-literal-error

Conversation

@sivchari

@sivchari sivchari commented Mar 17, 2026

Copy link
Copy Markdown
Member

Close #734
Close #879

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 17, 2026
@sivchari sivchari force-pushed the fix/marker-parser-numeric-literal-error branch from 7f58243 to 9c43c07 Compare March 27, 2026 03:02

@JoelSpeed JoelSpeed left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sivchari can you provide an example of where this bit you? Would be good to understand the motivation for this change before we accept it

@sivchari

Copy link
Copy Markdown
Member Author

I've not ran into this problem. I just try to fix #879 and #734.
Updated pr description.

Comment thread pkg/markers/parse_test.go
It("should support non-uniform, nested maps (and always guess as such)", argParseTestCase{arg: anyArg, raw: `{text: "abc", len: 3, "as bytes": 97;98;99, props: {encoding: ascii, nullsafe: true, tags: {"triple", "in a row"}}}`, output: complexMap}.Run)

It("should parse number-ish strings with invalid octal prefix as string", argParseTestCase{arg: anyArg, raw: `087bdd`, output: "087bdd"}.Run)
It("should parse path-like strings as string", argParseTestCase{arg: anyArg, raw: `/tmp/tmp.4paHkdEcpK`, output: "/tmp/tmp.4paHkdEcpK"}.Run)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add test for path-like case from #734 (e.g.,
"123/path") to ensure better coverage?

@camilamacedo86 camilamacedo86 Apr 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
It("should parse path-like strings as string", argParseTestCase{arg: anyArg, raw: `/tmp/tmp.4paHkdEcpK`, output: "/tmp/tmp.4paHkdEcpK"}.Run)
It("should parse path-like strings as string", argParseTestCase{arg: anyArg, raw: `/tmp/tmp.4paHkdEcpK`, output: "/tmp/tmp.4paHkdEcpK"}.Run)
It("should parse path-like strings as string", argParseTestCase{arg: anyArg, raw: `/tmp/tmp.4paHkdEcpK`, output: "/tmp/tmp.4paHkdEcpK"}.Run)
It("should parse numeric path as string", argParseTestCase{arg: anyArg, raw: `123/config`, output: "123/config"}.Run)
It("should parse invalid octal number as string", argParseTestCase{arg: anyArg, raw: `0987`, output: "0987"}.Run)
It("should parse resource quantity as string", argParseTestCase{arg: anyArg, raw: `100Mi`, output: "100Mi"}.Run)
It("should parse date-like string as string", argParseTestCase{arg: anyArg, raw: `2024-01-15`, output: "2024-01-15"}.Run)
It("should parse SHA prefix as string", argParseTestCase{arg: anyArg, raw: `sha256:08a7b`, output: "sha256:08a7b"}.Run)
It("should parse UUID-like string as string", argParseTestCase{arg: anyArg, raw: `00000000-1234-5678`, output: "00000000-1234-5678"}.Run)
It("should parse alphanumeric starting with number as string", argParseTestCase{arg: anyArg, raw: `123abc`, output: "123abc"}.Run)

@sivchari could you please add those ?
Then , I am have to LGTM this one.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx! I added these testcases.

@camilamacedo86 camilamacedo86 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just missing more test to cover all scenarios.
thank you for the contribution 🎉

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 27, 2026

@camilamacedo86 camilamacedo86 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 27, 2026
@k8s-ci-robot

Copy link
Copy Markdown
Contributor

LGTM label has been added.

DetailsGit tree hash: fcf7b683c6adaf359716e99ad7dd629ce2a76958

@camilamacedo86

Copy link
Copy Markdown
Member

Hi @sbueringer @JoelSpeed @alvaroaleman

This one I checked already.
It seems good to get merged.

Could you please give a look and see if we can get this one merged?

Signed-off-by: sivchari <shibuuuu5@gmail.com>
@sivchari sivchari force-pushed the fix/marker-parser-numeric-literal-error branch from 4ecab7f to bc6f44c Compare June 18, 2026 05:43
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 18, 2026

@camilamacedo86 camilamacedo86 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 18, 2026
@k8s-ci-robot

Copy link
Copy Markdown
Contributor

LGTM label has been added.

DetailsGit tree hash: a9549fcea968b86c39e0c12e2c2f3917965f1b32

@k8s-ci-robot

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: camilamacedo86, sivchari
Once this PR has been reviewed and has the lgtm label, please assign joelspeed for approval. For more information see the Code Review Process.

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

Details Needs approval from an approver in each of these files:

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

@camilamacedo86

Copy link
Copy Markdown
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jun 18, 2026
@alvaroaleman alvaroaleman added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jun 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Custom Flag/Arg Parsing Library Fails On Octal-looking Strings controller-gen interprets path as numeric literal and fails

5 participants