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.

Sign up for GitHub

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

Jump to bottom

✨ Infer version information for go-install #4516

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

✨ Infer version information for go-install #4516

wants to merge 1 commit into from

Conversation

migueleliasweb
Copy link
Contributor

@migueleliasweb migueleliasweb commented Jan 21, 2025 •
edited
Loading

Now that kubebuilder is go-installable 🎉, this is a small improvement to fetch more information when dealing with go install installations.

With this PR, we will now infer correctly GitCommit, BuildDate, GoOs and GoArch for normal GoReleaser builds but also go install installations.

Also a new field is added for goVersion

Outputs from make build and goreleaser are kept intact:

$ ./bin/kubebuilder version
Version: cmd.version{KubeBuilderVersion:"v4.5.1-10-gd7dd0cba4", KubernetesVendor:"1.32.1", GoVersion:"go1.23.4", GoOs:"linux", GoArch:"amd64", GitCommit:"d7dd0cba43a626e7febb19a3db6d8635b4c6d299", BuildDate:"2025-03-01T11:52:16Z"}

$ ./dist/kubebuilder_linux_amd64_v1/kubebuilder version
Version: cmd.version{KubeBuilderVersion:"4.5.1-SNAPSHOT-d7dd0cba4", KubernetesVendor:"1.32.1", GoVersion:"go1.23.4", GoOs:"linux", GoArch:"amd64", GitCommit:"d7dd0cba43a626e7febb19a3db6d8635b4c6d299", BuildDate:"2025-03-01T11:53:00Z"}

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 21, 2025
@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 21, 2025
@k8s-ci-robot
Copy link
Contributor

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

migueleliasweb
migueleliasweb commented Jan 21, 2025
View reviewed changes
dmvolod
dmvolod reviewed Jan 22, 2025
View reviewed changes
dmvolod
dmvolod reviewed Jan 22, 2025
View reviewed changes
dmvolod
dmvolod reviewed Jan 22, 2025
View reviewed changes
@camilamacedo86
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 23, 2025
@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 22, 2025
@migueleliasweb migueleliasweb mentioned this pull request Feb 27, 2025
Closed
@migueleliasweb migueleliasweb force-pushed the improve-version-go-install branch 4 times, most recently from 2a365ac to e3072e5 Compare March 1, 2025 04:17
@migueleliasweb
Copy link
Contributor Author

@dmvolod Simplified the version function now. Added a new goVersion. Let me know if you think that's okay.

@migueleliasweb migueleliasweb requested a review from dmvolod March 1, 2025 04:18
@migueleliasweb migueleliasweb force-pushed the improve-version-go-install branch from e3072e5 to d7dd0cb Compare March 1, 2025 11:51
@migueleliasweb migueleliasweb force-pushed the improve-version-go-install branch from d7dd0cb to 7e4c789 Compare March 1, 2025 12:06
camilamacedo86
camilamacedo86 reviewed Mar 1, 2025
View reviewed changes
cmd/version.go
goos = unknown
goarch = unknown
gitCommit = "$Format:%H$" // sha1 from git, output of $(git rev-parse HEAD)
kubernetesVendorVersion = "1.32.1"
Copy link
Member

Choose a reason for hiding this comment

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

Just to share, if you want to work on this one.

We can do that as a follow up, but we need a target to be called when we run amke generate that will update the k8s version here and in the goreleaser based on the k8s/api (like https://github.com/kubernetes-sigs/kubebuilder/blob/master/testdata/project-v4/Makefile#L179) so, that we can ensure that it is in sync always.

Copy link
Member

Choose a reason for hiding this comment

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

See: #4588

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I thought of that too while I was working on this file. I'll give it go.

Copy link
Contributor Author

@migueleliasweb migueleliasweb Mar 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Ops, it has been assigned already. All good.

camilamacedo86
camilamacedo86 approved these changes Mar 1, 2025
View reviewed changes
Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve
/ok-to-test

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 1, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, migueleliasweb

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 1, 2025
@migueleliasweb migueleliasweb changed the title ✨ [Improvement] Infer version information for go-install ✨ Infer version information for go-install Mar 1, 2025
dmvolod
dmvolod reviewed Mar 2, 2025
View reviewed changes
cmd/version.go
}

for _, setting := range info.Settings {
if buildDate == unknown && setting.Key == "vcs.revision" {
Copy link
Member

@dmvolod dmvolod Mar 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This should be a vcs.time getting buildDate, not vcs.revision

Also would be nice to have a minimal test for this case, as I described before mocking debug.ReadBuildInfo()

Copy link
Member

Choose a reason for hiding this comment

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

/hold until @dmvolod give LGTM
and the questions be answered

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@camilamacedo86 camilamacedo86 camilamacedo86 approved these changes

@dmvolod dmvolod dmvolod left review comments

@varshaprasad96 varshaprasad96 Awaiting requested review from varshaprasad96

Assignees

@camilamacedo86 camilamacedo86

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@migueleliasweb @k8s-ci-robot @camilamacedo86 @dmvolod