Skip to content

Fully ignore unknown tags in ParseCommentTags #519

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.

Sign up for GitHub

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

Jump to bottom
Merged
k8s-ci-robot merged 1 commit into from
Dec 12, 2024
Merged

Fully ignore unknown tags in ParseCommentTags #519

k8s-ci-robot merged 1 commit into from
Dec 12, 2024
+53 −3

Conversation

@jpbetz
Copy link
Contributor

@jpbetz jpbetz commented Dec 7, 2024 •
edited
Loading

Fix generators.ParseCommentTags to not fail on parsing on tags that it ignores anyway.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 7, 2024
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 7, 2024
@jpbetz jpbetz mentioned this pull request Dec 7, 2024
Merged
@jpbetz jpbetz changed the title Completely ignore unknown tags in generators.ParseCommentTags Add option to completely ignore unknown tags in generators.ParseCommentTags Dec 7, 2024
@jpbetz jpbetz changed the title Add option to completely ignore unknown tags in generators.ParseCommentTags Add option to completely ignore unknown tags in ParseCommentTags Dec 7, 2024
@jpbetz jpbetz force-pushed the ignore-unrecognized branch 2 times, most recently from 1dbbfa0 to ba9fe2a Compare December 8, 2024 16:22
@thockin
Copy link
Member

thockin commented Dec 9, 2024

I've made this opt-in since a full unit test run demonstrated that we have code that depends on the existing behavior.

Is that VALID, GOOD code or just code that exercised the existing behavior?

yongruilin
yongruilin reviewed Dec 9, 2024
View reviewed changes
pkg/generators/openapi_test.go
StringValue string
// +k8s:validation:maxitems=10
// +k8s:validation:maxItems=10
Copy link

@yongruilin yongruilin Dec 9, 2024

Choose a reason for hiding this comment

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

Wondering why this passed the test before?

Copy link
Contributor Author

@jpbetz jpbetz Dec 9, 2024

Choose a reason for hiding this comment

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

Because maxItems is never used to check anything in the test :( If the test had tried to exceed maxItems, it would have been allowed.

I could just remove this validation rules to simplify the test case..

Copy link

@yongruilin yongruilin Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

hmmm.. I think it is because the json.Unmarshal is not case-sensitive. So when unmarshal the commentTags here

kube-openapi/pkg/generators/markers.go

Line 411 in 9959940

if err = json.Unmarshal(out, &commentTags); err != nil {
, maxItems and maxitems both are parsed. So the test actually passes with here remain as maxitems.

How about having a TODO and a more comprehensive test by a separate PR? i'm thinking maybe need to have an option for the testOpenAPITypeWriter to cover "ignore unknown" in the openapi_test.go.

Copy link
Member

@thockin thockin Dec 10, 2024

Choose a reason for hiding this comment

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

We should not rely on that, I think the JSON we use in k/k is not case-insensitve

Copy link

@yongruilin yongruilin Dec 10, 2024

Choose a reason for hiding this comment

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

k/k is using https://github.com/kubernetes-sigs/json which is case sensitive

Copy link
Contributor Author

@jpbetz jpbetz Dec 11, 2024

Choose a reason for hiding this comment

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

I'll follow up on this in a separate issue.

@jpbetz jpbetz force-pushed the ignore-unrecognized branch from ba9fe2a to bb996bf Compare December 11, 2024 02:38
@jpbetz
Copy link
Contributor Author

jpbetz commented Dec 11, 2024

I've made this opt-in since a full unit test run demonstrated that we have code that depends on the existing behavior.

Is that VALID, GOOD code or just code that exercised the existing behavior?

I dug deeper. One of the problem cases was for the [ delimited strings Alex added support for with the multiline cel support, and the other was a check for empty string. I've fixed this to handle both and dropped the options.

@jpbetz jpbetz force-pushed the ignore-unrecognized branch from bb996bf to 2ce1b89 Compare December 11, 2024 02:43
@thockin
Copy link
Member

thockin commented Dec 11, 2024

Edit PR comment to remote words about options?

@jpbetz jpbetz changed the title Add option to completely ignore unknown tags in ParseCommentTags Fully ignore unknown tags in ParseCommentTags Dec 11, 2024
@jpbetz
Copy link
Contributor Author

jpbetz commented Dec 11, 2024

Edit PR comment to remote words about options?

Thanks. Yes, that's misleading. Fixed.

thockin
thockin reviewed Dec 11, 2024
View reviewed changes
Copy link
Member

@thockin thockin left a comment

Choose a reason for hiding this comment

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

LGTM

pkg/generators/markers.go
return nil
}

func isKnownTagCommentKey(key string) bool {
Copy link
Member

@thockin thockin Dec 11, 2024

Choose a reason for hiding this comment

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

Could you comment this - Would be nice to explain the expected "grammar" for a key here.

Copy link
Contributor Author

@jpbetz jpbetz Dec 11, 2024

Choose a reason for hiding this comment

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

Sure, added.

Copy link
Member

@thockin thockin Dec 11, 2024

Choose a reason for hiding this comment

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

not pushed?

Copy link
Contributor Author

@jpbetz jpbetz Dec 12, 2024

Choose a reason for hiding this comment

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

oops, pushed now

@jpbetz jpbetz force-pushed the ignore-unrecognized branch from 2ce1b89 to 04b9783 Compare December 12, 2024 01:41
@thockin
Copy link
Member

thockin commented Dec 12, 2024

Thanks!

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 12, 2024
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Dec 12, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jpbetz, thockin

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:

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 k8s-ci-robot merged commit 5ad02ce into kubernetes:master Dec 12, 2024
6 checks passed
@jpbetz jpbetz mentioned this pull request Dec 12, 2024
Merged
@Jefftree Jefftree mentioned this pull request Dec 13, 2024
Merged
@Jefftree Jefftree mentioned this pull request Mar 18, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexzielenski alexzielenski Awaiting requested review from alexzielenski

@seans3 seans3 Awaiting requested review from seans3

2 more reviewers

@thockin thockin thockin left review comments

@yongruilin yongruilin yongruilin left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@thockin thockin

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. 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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jpbetz @thockin @k8s-ci-robot @yongruilin