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.

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

internal: bump gatewayapi dep to latest #3603

Merged
merged 5 commits into from
Apr 27, 2021

Conversation

stevesloka
Copy link
Member

Updates the gateway-api dep to "latest" to keep on pace with breaking changes in the upcoming v0.3.0 release.

Fixes #3578

Signed-off-by: Steve Sloka slokas@vmware.com

@stevesloka stevesloka added this to the 1.15.0 milestone Apr 21, 2021
@stevesloka stevesloka requested a review from a team as a code owner April 21, 2021 15:41
@stevesloka stevesloka requested review from danehans and sunjayBhatia and removed request for a team April 21, 2021 15:41
@stevesloka
Copy link
Member Author

I tried to keep things into separate commits to make review easier, but the pointers, oh my. =)

@codecov
Copy link

codecov bot commented Apr 21, 2021

Codecov Report

Merging #3603 (0f5d4f9) into main (5f1cc94) will decrease coverage by 0.01%.
The diff coverage is 90.38%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3603      +/-   ##
==========================================
- Coverage   76.73%   76.72%   -0.02%     
==========================================
  Files         100      100              
  Lines        7097     7124      +27     
==========================================
+ Hits         5446     5466      +20     
- Misses       1534     1538       +4     
- Partials      117      120       +3     
Impacted Files Coverage Δ
internal/dag/gatewayapi_processor.go 90.56% <90.38%> (-2.41%) ⬇️

@skriss
Copy link
Member

skriss commented Apr 21, 2021

Looks like this also covers the k8s 1.21 bump that's also in #3581 - can either merge #3581 or close it

@stevesloka
Copy link
Member Author

We can merge #3581 first no problem. Let me go build out a 1.21 kind image to test with in the meantime until an official image is out.

Copy link
Member

@youngnick youngnick left a comment

Choose a reason for hiding this comment

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

This is looking great, and really illustrates why it's better to do this now, rather than later.

@skriss
Copy link
Member

skriss commented Apr 22, 2021

needs a rebase now

@stevesloka
Copy link
Member Author

I ended up squashing my original commits since I had to keep doing the entire rebase again. 😢

internal/dag/gatewayapi_processor.go Outdated Show resolved Hide resolved
internal/dag/gatewayapi_processor.go Outdated Show resolved Hide resolved
internal/dag/gatewayapi_processor.go Outdated Show resolved Hide resolved
internal/dag/gatewayapi_processor.go Outdated Show resolved Hide resolved
internal/dag/gatewayapi_processor.go Outdated Show resolved Hide resolved
// https://github.com/kubernetes-sigs/gateway-api/commit/9d63656df8cc9e67da60d4fe3f6289aad22e72d3
if match.Path.Value == "" || match.Path.Type == "" {
match.Path = gatewayapi_v1alpha1.HTTPPathMatch{Type: gatewayapi_v1alpha1.PathMatchPrefix, Value: "/"}
if match.Path == nil && match.Headers == nil {
Copy link
Member

Choose a reason for hiding this comment

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

nice short circuit, +1

Copy link
Member

Choose a reason for hiding this comment

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

I assume we have tests already for this case

match.Path = gatewayapi_v1alpha1.HTTPPathMatch{Type: gatewayapi_v1alpha1.PathMatchPrefix, Value: "/"}
if match.Path == nil && match.Headers == nil {
// No match conditions are defined, so default to 'PrefixMatch' with a value of '/'.
match.Path = &gatewayapi_v1alpha1.HTTPPathMatch{Type: pathMatchTypePtr(gatewayapi_v1alpha1.PathMatchPrefix), Value: pointer.StringPtr("/")}
Copy link
Member

Choose a reason for hiding this comment

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

should this modify mc insteadof match?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can actually remove this whole bit since now we're checking for nil in the other helper bits.

} else {
switch *match.Type {
case gatewayapi_v1alpha1.PathMatchPrefix:
mc.pathMatchConditions = append(mc.pathMatchConditions, &PrefixMatchCondition{Prefix: pointer.StringDeref(match.Value, "/")})
Copy link
Member

Choose a reason for hiding this comment

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

nit: could pull out the pointer.StringDeref(match.Value, "/") above so it is only done once

return nil
}

if match.Type != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Gotcha, I updated. We're finding some good bugs in this PR. =)

Signed-off-by: Steve Sloka <slokas@vmware.com>
Signed-off-by: Steve Sloka <slokas@vmware.com>
Signed-off-by: Steve Sloka <slokas@vmware.com>
Signed-off-by: Steve Sloka <slokas@vmware.com>
Signed-off-by: Steve Sloka <slokas@vmware.com>
Copy link
Member

@skriss skriss left a comment

Choose a reason for hiding this comment

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

LGTM, all the pointer stuff is definitely annoying but 🤷‍♂️

@stevesloka
Copy link
Member Author

Thanks for all the reviews @skriss @sunjayBhatia @youngnick! 🎉 I'll merge this now and refresh once the GatewayAPI release is out.

@stevesloka stevesloka merged commit d9a0d51 into projectcontour:main Apr 27, 2021
@stevesloka stevesloka deleted the issue/3578 branch April 27, 2021 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update GatewayAPI Reference to use SHA
4 participants