Skip to content
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

Add API for GEP-3171 #3283

Merged
merged 69 commits into from
Aug 27, 2024
Merged

Add API for GEP-3171 #3283

merged 69 commits into from
Aug 27, 2024

Conversation

jakebennert
Copy link
Contributor

@jakebennert jakebennert commented Aug 20, 2024 •
edited
Loading

What type of PR is this?
Add one of the following kinds:
/kind feature

What this PR does / why we need it:
This PR adds the API additions outlined in GEP 3171

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/feature Categorizes issue or PR as related to a new feature. labels Aug 20, 2024
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 20, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @jakebennert. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 20, 2024
@jakebennert jakebennert changed the title Implement API Changes for GEP-3171 Add API for GEP-3171 Aug 20, 2024
robscott
robscott reviewed Aug 20, 2024
View reviewed changes
Copy link
Member

@robscott robscott left a comment

Choose a reason for hiding this comment

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

Thanks @jakebennert!

/ok-to-test

apis/v1/httproute_types.go Outdated Show resolved Hide resolved
apis/v1/httproute_types.go Outdated Show resolved Hide resolved
apis/v1/shared_types.go Outdated Show resolved Hide resolved
apis/v1/shared_types.go Show resolved Hide resolved
apis/v1/httproute_types.go Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 20, 2024
jakebennert and others added 3 commits August 20, 2024 21:50
@jakebennert @robscott
Update apis/v1/httproute_types.go
205726a 
Co-authored-by: Rob Scott <rob.scott87@gmail.com>
@jakebennert @robscott
Update apis/v1/shared_types.go
dc6c3e4 
Co-authored-by: Rob Scott <rob.scott87@gmail.com>
@jakebennert
Add CEL validation to Fraction type
1b8f84d
@robscott robscott mentioned this pull request Aug 24, 2024
Closed
robscott
robscott reviewed Aug 26, 2024
View reviewed changes
Copy link
Member

@robscott robscott left a comment

Choose a reason for hiding this comment

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

Thanks @jakebennert! LGTM once GRPCRoute validation is covered

apis/v1/httproute_types.go Show resolved Hide resolved
pkg/test/cel/httproute_experimental_test.go Show resolved Hide resolved
robscott
robscott reviewed Aug 26, 2024
View reviewed changes
Copy link
Member

@robscott robscott left a comment

Choose a reason for hiding this comment

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

Thanks @jakebennert! A couple non-blocking nits on tests but otherwise LGTM.

/approve

pkg/test/cel/grcproute_experimental_test.go Outdated Show resolved Hide resolved
pkg/test/cel/grcproute_experimental_test.go Outdated Show resolved Hide resolved
pkg/test/cel/httproute_experimental_test.go Outdated Show resolved Hide resolved
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jakebennert, robscott

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 26, 2024
@jakebennert
Copy link
Contributor Author

/retest

@youngnick
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 27, 2024
@k8s-ci-robot k8s-ci-robot merged commit 45ab52e into kubernetes-sigs:main Aug 27, 2024
8 checks passed
kflynn
kflynn reviewed Aug 27, 2024
View reviewed changes
pkg/test/cel/grcproute_experimental_test.go
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be named grpcroute_... rather than grcproute_...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good catch - Is the easiest way to fix that to make another PR with that change?

kflynn
kflynn reviewed Aug 27, 2024
View reviewed changes
Copy link
Contributor

@kflynn kflynn left a comment

Choose a reason for hiding this comment

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

The substance of this looks great, thanks! There are a couple of typos, and I'd love to see some more test coverage, too, but overall nice work, many thanks! 🙂

pkg/test/cel/grcproute_experimental_test.go
////////////////////////////////////////////////////////////////////////////////
////////////////////////////////////////////////////////////////////////////////

func TestHTTPRequestMirrorFilterForGRPCRouteExperimental(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be TestGRPCRequestMirror...

@kflynn
Copy link
Contributor

kflynn commented Aug 27, 2024

@jakebennert Yeah, I think another PR is the way to go -- thanks!! 🙂 Do you think there's a way to easily add some tests to make sure that the non-error cases do something sane? 🙂

@jakebennert jakebennert mentioned this pull request Aug 27, 2024
Closed
@jakebennert
Copy link
Contributor Author

@jakebennert Yeah, I think another PR is the way to go -- thanks!! 🙂 Do you think there's a way to easily add some tests to make sure that the non-error cases do something sane? 🙂

#3300 - let me know what you think

This was referenced Aug 28, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kflynn kflynn kflynn left review comments

@robscott robscott robscott left review comments

@youngnick youngnick Awaiting requested review from youngnick

Assignees

@youngnick youngnick

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. kind/feature Categorizes issue or PR as related to a new feature. 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. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jakebennert @k8s-ci-robot @youngnick @kflynn @robscott