-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
🐛 Make kubebuilder go install-able #4481
base: master
Are you sure you want to change the base?
🐛 Make kubebuilder go install-able #4481
Conversation
Welcome @migueleliasweb! |
Hi @migueleliasweb. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
doc.go
Outdated
@@ -18,4 +18,4 @@ limitations under the License. | |||
|
|||
// Package kubebuilder contains pkged files compiled into the | |||
// go binaries. | |||
package kubebuilder | |||
package main |
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 couldn't find any references to this go:generate
command anywhere. Also github.com/markbates/pkger/cmd/pkger
is archived for a while.
Maybe this can be removed?
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.
Can we make this change?
We will need to check it further.
Because I believe that is what generates : https://pkg.go.dev/sigs.k8s.io/kubebuilder/v4
So, can we change the name of the package?
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'm really not sure that is generates anything right now. The package itself is not mentioned in go.mod
and the go generate
command fails for me.
Looking at the repo for Pcker
:
the code seems to be supposed to be a predecessor of go embed commands that are now part of the standard library.
Note that https://github.com/gobuffalo/packr is also archived in favour of go embed
native.
Have a look at the commit fe73ff7b99 and it seems it was initially tied to a command inside the alpha subcommand kubebuilder alpha config-gen
- this doesn't seem to be available now and I assume was renamed later to kubebuilder alpha generate
?
Sadly, changing the name of the package of doc.go
to kubebuilder
again would prevent the existance of the new main.go
as both files would have different package names.
If you really want to leave this file alone, there are still two alternatives that although much less nice, would mean we don't have to touch this file: #4079 (comment).
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'm not sure if this file must be present at the root, if not, it could be moved somewhere else?
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 will need to have some time to check this one properly.
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 created : #4504
if the docs are generated all fine, then we get it removed
if after we discover that it is used in some place and has a purpose, then we can quickly revert.
Thank you for highlighting that.
/ok-to-test |
You can see that the changes made broke the e2e tests. But all other failures would be required be solved |
a9a0a70
to
cb4d09d
Compare
I'll have a look at the failing tests tomorrow 🫡 |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: migueleliasweb, Scutua The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@@ -14,7 +14,7 @@ See the License for the specific language governing permissions and | |||
limitations under the License. | |||
*/ | |||
|
|||
package main | |||
package cmd |
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.
Seems to version getting form the info.Main.Version
should be adopted here like in the controller-tools project https://github.com/kubernetes-sigs/controller-tools/blob/main/pkg/version/version.go
Otherwise, the version printed after the go install ...
will be always unknown
.
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'm not sure if Kubebuilder itself tracks the same version as controller runtime. That might be a change in behaviour. If one of the maintainers wants to make that change, it's fine for me. scratch that.
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.
It's already done with the LD_FLAG on the build release stage and should be aligned here. Otherwise, for some cases it will be useless task, as we need to identify version on update or maintenance steps...
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.
@migueleliasweb can you please address this one ?
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'm not sure I fully understand what needs to be done here. Using debug.ReadBuildInfo()
might yield a change in behaviour compared to the current setup.
Would it be okay to address this in a separate PR? (I'm just trying to make this one specific for the go-installable
feature for kubebuilder.
If we still want to move ahead, I'm happy to mimic the same logic as in https://github.com/kubernetes-sigs/controller-tools/blob/main/pkg/version/version.go.
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.
Is this what we're looking for?
If the kubeBuilderVersion
is not set, fallback to info.Main.Version
.
// versionString returns the CLI version
func versionString() string {
+ if kubeBuilderVersion == "" {
+ if info, ok := debug.ReadBuildInfo(); ok && info.Main.Version != "" {
+ kubeBuilderVersion = info.Main.Version
+ }
+ }
+
return fmt.Sprintf("Version: %#v", version{
kubeBuilderVersion,
kubernetesVendorVersion,
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.
Pushed a change about this. Let me know if it looks good so I can squash everything before the merge. 🫡
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 think to get this one merged we just need:
a) address this comment https://github.com/kubernetes-sigs/kubebuilder/pull/4481/files#r1920699959.
b) can you please squash the commits? we have the policy of 1 commit per PR
c) we should get this one merged #4504 and then this one here rebased with so you will add the main.go in the root. If we need to revert for any reason that will be helpful, thank you a lot 🥇
Than you a lot for your help here.
Signed-off-by: Miguel Elias dos Santos <[email protected]>
a1dee55
to
21ffbec
Compare
Co-authored-by: Dmitry Volodin <[email protected]>
@migueleliasweb: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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-sigs/prow repository. I understand the commands that are listed here. |
Closes #4079
This PR makes
kubebuilder
go installable.Changes:
main
package originally fromcmd/
tocmd
main
function previously insidecmd/cmg.go
toRun
- following Go Cobra's patterndoc.go
tomain
make build
target