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
Use Kubernetes 1.16.2 #403
Use Kubernetes 1.16.2 #403
Conversation
703fc8e
to
738ea45
Compare
/retest |
1 similar comment
/retest |
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit on the dependency magnet
@@ -0,0 +1,15 @@ | |||
// +build tools | |||
|
|||
package tools |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The convention in other repos has been to use the package name dependencymagnet
, with filename doc.go
. Other contributors may misinterpret tools
and add real "tooling" go code.
The package should also have a godoc comment that indicates this is for adding non-go dependencies. See https://github.com/openshift/cluster-openshift-controller-manager-operator/blob/master/dependencymagnet/doc.go as an example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we invent our own convention? https://github.com/golang/go/wiki/Modules#how-can-i-track-tool-dependencies-for-a-module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/search?q=org%3Aopenshift+filename%3Atools.go+%22%2Bbuild+tools%22
And I'd prefer to move this file to hack/codegen and keep script dependencies near the scripts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a comment to align with other repos
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dmage I didn't realize there was an existing convention. The apiserver team seems to have adopted a different convention. Now that I am aware of the tools
convention, let's keep it.
https://github.com/search?q=org%3Aopenshift+filename%3Adoc.go+%22%2Bbuild+tools%22
go mod init : remove versions for replace in go.mod : use sigs.k8s.io/controller-tools v0.2.1 : use github.com/prometheus/client_golang v0.9.2 : use github.com/prometheus/common v0.4.1 go mod tidy go mod vendor
e6c03b6
to
d7ba0f8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adambkaplan, dmage The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest Please review the full test history for this PR and help us cut down flakes. |
No description provided.