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

*: Add initial 1.24 tool support #10707

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

timflannagan
Copy link
Member

@timflannagan timflannagan commented Feb 26, 2025

Description

Closes #10645.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

@timflannagan timflannagan force-pushed the chore/1.24-tool-support branch 3 times, most recently from dd48d1f to 10fcc29 Compare February 26, 2025 19:54
Signed-off-by: timflannagan <[email protected]>
@@ -48,8 +47,8 @@ runs:
IMAGE_VARIANT: ${{ matrix.image-variant }}
CONFORMANCE: "true"
CONFORMANCE_VERSION: ${{ env.CONFORMANCE_VERSION }}

run: ./hack/kind/setup-kind.sh
run: make tool/kind && ./hack/kind/setup-kind.sh
Copy link
Member Author

Choose a reason for hiding this comment

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

Need to open an issue for this. We should be using a makefile target instead of calling bash scripts directly for the majority of our jobs.

Comment on lines -329 to -331
.PHONY: generate-crd-reference-docs
generate-crd-reference-docs:
go run docs/content/crds/generate.go
Copy link
Member Author

Choose a reason for hiding this comment

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

Removed the generate-crd-reference-docs as it's currently broken. See #10568.

.PHONY: generate-crd-reference-docs
generate-crd-reference-docs:
go run docs/content/crds/generate.go
getter-check: tool/gettercheck ## Runs all generate directives for mockgen in the repo
Copy link
Member Author

Choose a reason for hiding this comment

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

Unclear whether we wanted to migrate this tool, but I thought there was an open issue for removing it or replacing it with the protogetter golangci-lint linter. I can create that issue if we don't want to rip out this tool here.

@@ -573,14 +621,16 @@ endif # distroless images
CLUSTER_NAME ?= kind
INSTALL_NAMESPACE ?= kgateway-system

kind-setup:
KIND ?= go tool sigs.k8s.io/kind
Copy link
Member Author

Choose a reason for hiding this comment

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

Added kind as a tool as we rely on it for a portion of the testing matrix. I realize we may want to add envtest as a tool too...

Comment on lines -6 to +13
kubectl rollout status -n metallb-system deployment/controller --timeout 2m
kubectl rollout status -n metallb-system daemonset/speaker --timeout 2m
kubectl rollout status -n metallb-system deployment/controller --timeout 5m
kubectl rollout status -n metallb-system daemonset/speaker --timeout 5m
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 were seeing timeouts in a couple of recent PR runs. Wasn't able to reproduce locally.

sigs.k8s.io/gateway-api v1.2.1
sigs.k8s.io/structured-merge-diff/v4 v4.5.0
sigs.k8s.io/yaml v1.4.0
)

require (
4d63.com/gocheckcompilerdirectives v1.2.1 // indirect
Copy link
Contributor

Choose a reason for hiding this comment

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

this adds a lot of new deps to go.mod.
should we consider creating a separate go.mod just for the tools?

Copy link
Contributor

Choose a reason for hiding this comment

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

These additions shouldn't interfere with deps for the main module according to go docs on the tool feature

@nfuden
Copy link
Contributor

nfuden commented Feb 26, 2025

should we use the tools.go thing introduced in 1.24? https://pkg.go.dev/golang.org/x/tools

# Tools
#----------------------------------------------------------------------------

.PHONY: tools
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 tools are saved in the go.mod, so we shouldn't need them in the makefile

#----------------------------------------------------------------------------------
# Repo setup
#----------------------------------------------------------------------------------

GOIMPORTS ?= go tool golang.org/x/tools/cmd/goimports
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: you can use short form go tool goimports

Copy link
Contributor

@yuval-k yuval-k left a comment

Choose a reason for hiding this comment

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

You can delete ci/tools.go

@timflannagan
Copy link
Member Author

You can delete ci/tools.go

@yuval-k We moved the ci/ directory to hack/ a couple of weeks back if you're looking at you're looking at an older branch. The hacks/tools.go file was removed with these changes

@timflannagan timflannagan added the work in progress signals bulldozer to keep pr open (don't auto-merge) label Mar 3, 2025
@timflannagan
Copy link
Member Author

Adding the WIP label. I'll get back to this sometime this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
work in progress signals bulldozer to keep pr open (don't auto-merge)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adopt Go 1.24 Changes such as tools changes
4 participants