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

chore: use the same tag in Makefile as in code when installing Gateway API CRDs #2567

Merged

Conversation

pmalek
Copy link
Member

@pmalek pmalek commented Jun 13, 2022

What this PR does / why we need it:

In order to use the same version of Gateway API CRDs when installing them via Makefile as in the Go code.

This PR adds a make target generate.gateway-api-crds-url that generates test/consts/gateway.go (from test/internal/cmd/generate-gateway-api-crds-url/gateway_consts.tmpl) using ./test/internal/cmd/generate-gateway-api-crds-url/main.go.

If one would like to override the version then it's as simple as adding GATEWAY_API_VERSION to the make target invocation like so:

GATEWAY_API_VERSION=v0.4.1 make generate.gateway-api-crds-url

Related PR that fixed it in Go code: #2551

@pmalek pmalek self-assigned this Jun 13, 2022
@pmalek pmalek marked this pull request as ready for review June 13, 2022 13:16
@pmalek pmalek requested a review from a team as a code owner June 13, 2022 13:16
@pmalek pmalek temporarily deployed to Configure ci June 13, 2022 13:16 Inactive
@pmalek pmalek temporarily deployed to Configure ci June 13, 2022 13:16 Inactive
@pmalek pmalek temporarily deployed to Configure ci June 13, 2022 13:40 Inactive
Copy link
Contributor

@mflendrich mflendrich left a comment

Choose a reason for hiding this comment

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

Bitrot alert. go.mod today references v0.4.1-0.20220306235253-71fee1c2808f and it's guaranteed that discrepancies between the version in go.mod, in the makefile and in the test as changed in #2551 will keep reappearing.

Can we do something to have a single source of truth for the gateway api version?
Maybe, rather than referring to a specific release from github, we can kustomize build the kustomization already downloaded to $(go env GOPATH)/pkg/mod/sigs.k8s.io/[email protected] ? Or somehow else ensure that the version in go.mod is authoritative.

Maybe that's an excess of form over content, just wanted to bring this up for your consideration.

@pmalek pmalek force-pushed the makefile-use-the-same-gateway-api-tag-when-installing-crds branch from 4cce278 to 242ad67 Compare June 13, 2022 14:14
@pmalek pmalek temporarily deployed to Configure ci June 13, 2022 14:14 Inactive
@pmalek pmalek temporarily deployed to Configure ci June 13, 2022 14:14 Inactive
@pmalek
Copy link
Member Author

pmalek commented Jun 13, 2022

sigs.k8s.io/gateway-api

Agreed, and that's actually a great idea. PTAL at what's proposed in https:/Kong/kubernetes-ingress-controller/pull/2567/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52

@pmalek pmalek temporarily deployed to Configure ci June 13, 2022 14:40 Inactive
@pmalek pmalek temporarily deployed to Configure ci June 13, 2022 14:40 Inactive
@pmalek pmalek temporarily deployed to Configure ci June 13, 2022 15:01 Inactive
@pmalek pmalek force-pushed the makefile-use-the-same-gateway-api-tag-when-installing-crds branch from 242ad67 to 9a8db1d Compare June 14, 2022 08:14
@pmalek pmalek temporarily deployed to Configure ci June 14, 2022 08:14 Inactive
@pmalek pmalek temporarily deployed to Configure ci June 14, 2022 08:15 Inactive
@pmalek pmalek temporarily deployed to Configure ci June 14, 2022 08:36 Inactive
@shaneutt shaneutt requested a review from mflendrich June 14, 2022 13:38
Copy link
Contributor

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

How would you feel about changing the code in internal/util/test/crds.go from the previous PR to accept the version from the environment, so that we don't have to update that going forward?

@pmalek
Copy link
Member Author

pmalek commented Jun 14, 2022

How would you feel about changing the code in internal/util/test/crds.go from the previous PR to accept the version from the environment, so that we don't have to update that going forward?

Do you have an environment variable in mind or something else? What we could do is to make a separate file for that URL and add a go generate clause which would pull the same info as in the Makefile (go list -m -f ...) and put it in the const. WDYT?

@shaneutt
Copy link
Contributor

How would you feel about changing the code in internal/util/test/crds.go from the previous PR to accept the version from the environment, so that we don't have to update that going forward?

Do you have an environment variable in mind or something else? What we could do is to make a separate file for that URL and add a go generate clause which would pull the same info as in the Makefile (go list -m -f ...) and put it in the const. WDYT?

I'm not opposed to making it a generated var, however this does preclude the notion that someone could seed the value with an environment variable, so maybe just do a combination of both?

@pmalek pmalek force-pushed the makefile-use-the-same-gateway-api-tag-when-installing-crds branch from 9a8db1d to 544ad57 Compare June 15, 2022 12:57
@pmalek pmalek temporarily deployed to Configure ci June 15, 2022 12:57 Inactive
@pmalek pmalek force-pushed the makefile-use-the-same-gateway-api-tag-when-installing-crds branch from 544ad57 to 9c01b1c Compare June 15, 2022 12:58
@pmalek pmalek temporarily deployed to Configure ci June 15, 2022 12:58 Inactive
@pmalek pmalek requested a review from shaneutt June 15, 2022 12:58
@pmalek pmalek force-pushed the makefile-use-the-same-gateway-api-tag-when-installing-crds branch from 9c01b1c to 2e32784 Compare June 15, 2022 13:00
@pmalek pmalek force-pushed the makefile-use-the-same-gateway-api-tag-when-installing-crds branch from 9a16ea1 to 100b8e0 Compare June 15, 2022 14:50
@pmalek pmalek temporarily deployed to Configure ci June 15, 2022 14:50 Inactive
@pmalek pmalek temporarily deployed to Configure ci June 15, 2022 14:50 Inactive
@pmalek pmalek temporarily deployed to Configure ci June 15, 2022 15:14 Inactive
@pmalek pmalek force-pushed the makefile-use-the-same-gateway-api-tag-when-installing-crds branch from 100b8e0 to 81720bc Compare June 15, 2022 15:31
@pmalek pmalek force-pushed the makefile-use-the-same-gateway-api-tag-when-installing-crds branch from 81720bc to 8679215 Compare June 15, 2022 15:32
@pmalek pmalek temporarily deployed to Configure ci June 15, 2022 15:33 Inactive
@pmalek pmalek temporarily deployed to Configure ci June 15, 2022 15:36 Inactive
@pmalek pmalek temporarily deployed to Configure ci June 15, 2022 16:00 Inactive
@pmalek pmalek force-pushed the makefile-use-the-same-gateway-api-tag-when-installing-crds branch from 8679215 to be6366f Compare June 21, 2022 13:02
@pmalek pmalek temporarily deployed to Configure ci June 21, 2022 13:02 Inactive
@pmalek pmalek temporarily deployed to Configure ci June 21, 2022 13:35 Inactive
@pmalek pmalek enabled auto-merge (squash) June 21, 2022 16:58
Makefile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
shaneutt
shaneutt previously approved these changes Jun 22, 2022
Copy link
Contributor

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

I would just like to see us put the link to the TODO follow up into the Makefile and then lgtm

@pmalek pmalek force-pushed the makefile-use-the-same-gateway-api-tag-when-installing-crds branch from be6366f to d4bffb2 Compare June 22, 2022 15:07
@pmalek pmalek temporarily deployed to Configure ci June 22, 2022 15:07 Inactive
@pmalek pmalek temporarily deployed to Configure ci June 22, 2022 15:07 Inactive
@shaneutt shaneutt self-requested a review June 22, 2022 15:10
@pmalek pmalek merged commit 7d5108a into main Jun 22, 2022
@pmalek pmalek temporarily deployed to Configure ci June 22, 2022 15:30 Inactive
@pmalek pmalek deleted the makefile-use-the-same-gateway-api-tag-when-installing-crds branch June 22, 2022 15:30
pmalek added a commit that referenced this pull request Jun 24, 2022
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.

3 participants