Skip to content

Drop tools.go in favor of native tool directive support in go 1.24 #535

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

Merged
merged 2 commits into from
Jun 11, 2025

Conversation

fmuyassarov
Copy link
Collaborator

No description provided.

github: strip away v prefix during bundle submission

Signed-off-by: Feruzjon Muyassarov <[email protected]>
@klihub
Copy link
Collaborator

klihub commented Jun 10, 2025

@fmuyassarov I'm not against this at all, but could we delay this until we manage to get most of the other functional PR's merged ? We have a bit of a crunch going on ATM, so I'd prefer to focus on getting those merged first.

@fmuyassarov
Copy link
Collaborator Author

@fmuyassarov I'm not against this at all, but could we delay this until we manage to get most of the other functional PR's merged ? We have a bit of a crunch going on ATM, so I'd prefer to focus on getting those merged first.

Absolutely. You can ping me here whenever you will ready for this change. Thanks.

@fmuyassarov fmuyassarov marked this pull request as draft June 10, 2025 19:48
@klihub
Copy link
Collaborator

klihub commented Jun 11, 2025

@fmuyassarov I'm not against this at all, but could we delay this until we manage to get most of the other functional PR's merged ? We have a bit of a crunch going on ATM, so I'd prefer to focus on getting those merged first.

Absolutely. You can ping me here whenever you will ready for this change. Thanks.

@fmuyassarov Then again, if those CI failures would go away by simply bumping golang to 1.24.x in go.mod, then there is no need to delay this. We can do so and merge this once it is updated.

@klihub
Copy link
Collaborator

klihub commented Jun 11, 2025

@fmuyassarov I'm not against this at all, but could we delay this until we manage to get most of the other functional PR's merged ? We have a bit of a crunch going on ATM, so I'd prefer to focus on getting those merged first.

Absolutely. You can ping me here whenever you will ready for this change. Thanks.

@fmuyassarov Then again, if those CI failures would go away by simply bumping golang to 1.24.x in go.mod, then there is no need to delay this. We can do so and merge this once it is updated.

I pulled this in and it looks like the problems go away by a

go get k8s.io/client-go/tools/[email protected] && \
    go mod tidy && \
    git commit -m -s "go.{mod,sum} update and tidy deps."

After that make verify-generate verify-build verify-godeps && echo PASS passes locally.

Signed-off-by: Feruzjon Muyassarov <[email protected]>
@fmuyassarov fmuyassarov requested a review from klihub June 11, 2025 06:18
@fmuyassarov fmuyassarov marked this pull request as ready for review June 11, 2025 06:18
@klihub klihub requested review from askervin and marquiz June 11, 2025 06:59
Copy link
Collaborator

@marquiz marquiz left a comment

Choose a reason for hiding this comment

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

Thanks @fmuyassarov. The PR also updates the deps (especially k8s.io/code-generator). That's fine assuming you've tested that code-generation still works :) Suggestion: that could be mentioned in the commit message.

LGTM from me

@fmuyassarov
Copy link
Collaborator Author

fmuyassarov commented Jun 11, 2025

Thanks @fmuyassarov. The PR also updates the deps (especially k8s.io/code-generator). That's fine assuming you've tested that code-generation still works :) Suggestion: that could be mentioned in the commit message.

Generate target was ran successfully. I assume it is safe to bump the generator library.

make generate
/Users/est/work/nri-plugins/build-aux/bin/controller-gen \
		crd:generateEmbeddedObjectMeta=true output:crd:artifacts:config=config/crd/bases\
		paths="./pkg/apis/..."
/Users/est/work/nri-plugins/build-aux/bin/controller-gen \
		rbac:roleName=manager-role crd paths="./pkg/apis/..." output:crd:artifacts:config=config/crd/bases
/Users/est/work/nri-plugins/build-aux/bin/controller-gen object:headerFile="scripts/hack/boilerplate.go.txt" paths="./pkg/apis/..."
./scripts/hack/update_codegen.sh go /Users/est/work/nri-plugins/build-aux
Cleaning up temporary vendor directory...
Updating Helm chart CRDs for topology-aware plugin...
Updating Helm chart CRDs for balloons plugin...
Updating Helm chart CRDs for template plugin...
No generated CRD found for memory-qos plugin...
No generated CRD found for memtierd plugin...
No generated CRD found for sgx-epc plugin...

@klihub
Copy link
Collaborator

klihub commented Jun 11, 2025

Thanks @fmuyassarov. The PR also updates the deps (especially k8s.io/code-generator). That's fine assuming you've tested that code-generation still works :) Suggestion: that could be mentioned in the commit message.

Generate target was ran successfully. I assume it is safe to bump the generator library.

make generate
/Users/est/work/nri-plugins/build-aux/bin/controller-gen \
		crd:generateEmbeddedObjectMeta=true output:crd:artifacts:config=config/crd/bases\
		paths="./pkg/apis/..."
/Users/est/work/nri-plugins/build-aux/bin/controller-gen \
		rbac:roleName=manager-role crd paths="./pkg/apis/..." output:crd:artifacts:config=config/crd/bases
/Users/est/work/nri-plugins/build-aux/bin/controller-gen object:headerFile="scripts/hack/boilerplate.go.txt" paths="./pkg/apis/..."
./scripts/hack/update_codegen.sh go /Users/est/work/nri-plugins/build-aux
Cleaning up temporary vendor directory...
Updating Helm chart CRDs for topology-aware plugin...
Updating Helm chart CRDs for balloons plugin...
Updating Helm chart CRDs for template plugin...
No generated CRD found for memory-qos plugin...
No generated CRD found for memtierd plugin...
No generated CRD found for sgx-epc plugin...

Yes, there is a verify-generate target/CI job which runs that and checks that there are no changes. That having passed should mean that the generator has verbatim generated the same content as already committed to the repo, so it should be fine.

@klihub klihub merged commit dec6e0c into containers:main Jun 11, 2025
8 checks passed
@fmuyassarov fmuyassarov deleted the tools-directive branch June 11, 2025 13:59
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