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 controller tools back #293

Merged

Conversation

jensfr
Copy link
Contributor

@jensfr jensfr commented Apr 20, 2023

  • Description of the problem which is fixed/What is the use case
    Downstream Red Hat builds require controller-tools to be in the go.mod so they can be cached due to network limitations. They can easily be removed accidentally with go mod tidy because they are not actually dependencies.

  • What I did
    go get sigs.k8s.io/controller-tools@v0.9.2
    go get sigs.k8s.io/controller-tools/pkg/genall/help/pretty@v0.9.2
    go get sigs.k8s.io/controller-tools/pkg/crd@v0.9.2
    Edit: had to also do this (Greg)
    go get sigs.k8s.io/controller-tools/cmd/controller-gen@v0.9.2

  • How to verify it
    go run sigs.k8s.io/controller-tools/cmd/controller-gen object:headerFile="hack/boilerplate.go.txt" paths="./...""

  • Description for the changelog

@jensfr jensfr requested review from cpmeadors and removed request for littlejawa April 20, 2023 20:04
@cpmeadors cpmeadors changed the base branch from main to peer-pods-tech-preview April 20, 2023 20:21
@cpmeadors
Copy link
Contributor

I did a go mod download and then ran

go run sigs.k8s.io/controller-tools/cmd/controller-gen object:headerFile="hack/boilerplate.go.txt" paths="./..."

and got

../../../../go/pkg/mod/sigs.k8s.io/controller-tools@v0.9.2/cmd/controller-gen/main.go:25:2: missing go.sum entry for module providing package github.com/spf13/cobra (imported by sigs.k8s.io/controller-tools/cmd/controller-gen); to add:
go get sigs.k8s.io/controller-tools/cmd/controller-gen@v0.9.2

This is from a brand new clone of sandboxed-container-operator with the PR branch checked out. Either something changed or I missed a go get command the last time. Not sure why we didn't see this previously. Maybe everyone had a dirty test env.

@jensfr
Copy link
Contributor Author

jensfr commented Apr 20, 2023

I don't see that problem, even with a fresh checkout

@openshift-ci
Copy link

openshift-ci bot commented Apr 20, 2023

@jensfr: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/sandboxed-containers-operator-e2e 4560e24 link false /test sandboxed-containers-operator-e2e

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Copy link
Contributor

@bpradipt bpradipt 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 @jensfr

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 21, 2023
@bpradipt
Copy link
Contributor

Added a reference issue to fix it in code - #294

@gkurz
Copy link
Member

gkurz commented Apr 21, 2023

I did a go mod download and then ran

go run sigs.k8s.io/controller-tools/cmd/controller-gen object:headerFile="hack/boilerplate.go.txt" paths="./..."

and got

../../../../go/pkg/mod/sigs.k8s.io/controller-tools@v0.9.2/cmd/controller-gen/main.go:25:2: missing go.sum entry for module providing package github.com/spf13/cobra (imported by sigs.k8s.io/controller-tools/cmd/controller-gen); to add: go get sigs.k8s.io/controller-tools/cmd/controller-gen@v0.9.2

I hit exactly the same error and if I do go get sigs.k8s.io/controller-tools/cmd/controller-gen@v0.9.2 as suggested in the error then the test succeeds. This go get command simply adds github.com/inconshreveable/mousetrap and github.com/spf13/cobra as indirect dependencies. @jensfr do you see them in your go.mod ?

This is from a brand new clone of sandboxed-container-operator with the PR branch checked out. Either something changed or I missed a go get command the last time. Not sure why we didn't see this previously. Maybe everyone had a dirty test env.

@cpmeadors I've reset hard to the merge commit of #286 :

$ git reset --hard e54c02c47f05016f294113bce66d48f8af2909b4
HEAD is now at e54c02c47f05 Merge pull request #286 from cpmeadors/add-back-controller-tools-for-downstream-builds
$ git grep -E 'cobra|mouse' go.mod
go.mod: github.com/inconshreveable/mousetrap v1.0.1 // indirect
go.mod: github.com/spf13/cobra v1.6.0 // indirect

It seems that the extra go get wasn't needed back then but it is now because 6577693 removed them.

@gkurz gkurz added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 21, 2023
@jensfr
Copy link
Contributor Author

jensfr commented Apr 21, 2023

Travelling back today and probably won't get to this. Feel free to send another PR to fix this

Signed-off-by: Jens Freimann <jfreimann@redhat.com>
[greg: run go get sigs.k8s.io/controller-tools/cmd/controller-gen@v0.9.2 ]
Signed-off-by: Greg Kurz <groug@kaod.org>
@gkurz gkurz force-pushed the add-controller-tools-back branch from 4560e24 to c57f854 Compare April 21, 2023 13:03
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 21, 2023
@openshift-ci
Copy link

openshift-ci bot commented Apr 21, 2023

New changes are detected. LGTM label has been removed.

@gkurz
Copy link
Member

gkurz commented Apr 21, 2023

Travelling back today and probably won't get to this. Feel free to send another PR to fix this

Since your PR has Maintainers are allowed to edit this pull request., I could simply force-push the fix to your branch 😉

@gkurz gkurz requested a review from pmores April 21, 2023 13:07
@gkurz gkurz removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 21, 2023
@jensfr
Copy link
Contributor Author

jensfr commented Apr 21, 2023

Nice, thank you @gkurz

@gkurz gkurz merged commit 611d9f9 into openshift:peer-pods-tech-preview Apr 21, 2023
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.

None yet

4 participants