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

Updated Gateway API from v1alpha2 to v1beta1 #1319

Merged
merged 1 commit into from
Nov 23, 2022
Merged

Conversation

cgrotz
Copy link
Contributor

@cgrotz cgrotz commented Nov 20, 2022

This introduces v1beta1 and keeps support for v1alpha2.

The e2e testes where updated to Gateway API v1beta1 and Contour v1.23.0.

Fix: #1318

@aryan9600
Copy link
Member

aryan9600 commented Nov 22, 2022

@cgrotz thanks for opening this PR! please squash your commits into one, and sign it off.

i understand your reasoning, but given where the ecosystem is at, i think it'd be wise to support for v1alpha2 for some more time. cc: @stefanprodan

@cgrotz
Copy link
Contributor Author

cgrotz commented Nov 22, 2022

@cgrotz thanks for opening this PR! please squash your commits into one, and sign it off.

i understand your reasoning, but given where the ecosystem is at, i think it'd be wise to support for v1alpha2 for some more time. cc: @stefanprodan

Sure, IMHO I was assuming since v1alpha2 is deprecated for some months now, and Contour and GKE switched to v1beta1 that it would be okay to drop v1alpha2. I'll take a look at implementing it backwards compatible later today.

@stefanprodan
Copy link
Member

@cgrotz thanks for this PR. We can't remove the Gateway API v1alpha2 and force Flagger users into upgrading right away. As with SMI, we need to keep the alpha versions around, announce the deprecation and drop support after 6 months.

@cgrotz
Copy link
Contributor Author

cgrotz commented Nov 22, 2022

I made it backwards compatible, but I had to create a new configuration struct for flaggerv1.

I'm also not super fond of the var blocks at the top of gateway_api.go and gateway_api_v1beta1.go do you have an idea how to make that nicer?

Copy link
Member

@aryan9600 aryan9600 left a comment

Choose a reason for hiding this comment

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

instead of creating a new struct, lets reuse v1beta.ParentReference

pkg/apis/flagger/v1beta1/canary.go Outdated Show resolved Hide resolved
pkg/router/gateway_api.go Outdated Show resolved Hide resolved
pkg/router/gateway_api_v1beta1.go Outdated Show resolved Hide resolved
pkg/router/gateway_api_v1beta1.go Outdated Show resolved Hide resolved
pkg/router/gateway_api_v1beta1.go Outdated Show resolved Hide resolved
pkg/router/router_test.go Outdated Show resolved Hide resolved
@aryan9600
Copy link
Member

I'm also not super fond of the var blocks at the top of gateway_api.go and gateway_api_v1beta1.go do you have an idea how to make that nicer?

not that i'm aware of, golang doesn't allow references to constants, hence all the aliasing

@aryan9600
Copy link
Member

@cgrotz lets switch to v1beta1 in CI, could you update the contour version in the e2e tests as well?

@cgrotz
Copy link
Contributor Author

cgrotz commented Nov 22, 2022

@aryan9600 I changed it to v1beta1.ParentReference thanks for the idea. And updated the contour version to v1.23.0. Gatewayapi test runs for me (on GitHub Actions).

Copy link
Member

@aryan9600 aryan9600 left a comment

Choose a reason for hiding this comment

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

linters are failing, please run go fmt ./... from the root of the project

pkg/router/router_test.go Outdated Show resolved Hide resolved
@cgrotz
Copy link
Contributor Author

cgrotz commented Nov 22, 2022

@aryan9600

linters are failing, please run go fmt ./... from the root of the project
done, also fixed the test.

@cgrotz
Copy link
Contributor Author

cgrotz commented Nov 22, 2022

@aryan9600 the linter is still failing, I just ran go fmt again (on the whole folder and the individual files) but it doesn't change the file. What do you recommend?

@aryan9600
Copy link
Member

aryan9600 commented Nov 22, 2022

are you sure you the command ran properly? this is what i get on my machine:

❯ gh pr checkout 1319
Switched to branch 'cgrotz/main'

❯ make test-fmt
gofmt -l -s ./ | grep ".*\.go"; if [ "$?" = "0" ]; then exit 1; fi
pkg/apis/gatewayapi/v1beta1/httproute_types.go
pkg/apis/gatewayapi/v1beta1/shared_types.go
pkg/client/clientset/versioned/fake/register.go
pkg/client/clientset/versioned/scheme/register.go
make: *** [Makefile:20: test-fmt] Error 1

❯ go fmt ./...
pkg/apis/gatewayapi/v1beta1/httproute_types.go
pkg/apis/gatewayapi/v1beta1/shared_types.go
pkg/client/clientset/versioned/fake/register.go
pkg/client/clientset/versioned/scheme/register.go

❯ make test-fmt
gofmt -l -s ./ | grep ".*\.go"; if [ "$?" = "0" ]; then exit 1; fi
goimports -l ./ | grep ".*\.go"; if [ "$?" = "0" ]; then exit 1; fi

@cgrotz
Copy link
Contributor Author

cgrotz commented Nov 22, 2022

make test-fmt(base)
Yes it completes for without warning

> make test-fmt
gofmt -l -s ./ | grep ".*\.go"; if [ "$?" = "0" ]; then exit 1; fi
goimports -l ./ | grep ".*\.go"; if [ "$?" = "0" ]; then exit 1; fi

with go version go1.18.1 darwin/arm64

Signed-off-by: Christoph Grotz <grotz@google.com>
@cgrotz
Copy link
Contributor Author

cgrotz commented Nov 22, 2022

updated to go 1.19, now go fmt changed the files

@aryan9600 can you rerun the build? I think it should pass now.

@cgrotz cgrotz requested review from aryan9600 and removed request for stefanprodan November 22, 2022 14:59
Copy link
Member

@aryan9600 aryan9600 left a comment

Choose a reason for hiding this comment

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

LGTM! thanks a lot @cgrotz 🏅 🙇

@stefanprodan stefanprodan added the kind/enhancement Improvement request for an existing feature label Nov 23, 2022
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @cgrotz 🥇

@stefanprodan stefanprodan merged commit fce46e2 into fluxcd:main Nov 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Improvement request for an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gateway API Beta support
3 participants