Fix version tag management #5463

chansuke · 2023-11-23T15:59:00Z

k8s-ci-robot · 2023-11-23T15:59:10Z

Hi @chansuke. 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/test-infra repository.

stormqueen1990 · 2023-11-23T21:14:25Z

Hi there, @chansuke! 👋

Thanks for your contribution!

I see this PR is still a work in progress. Would you like a preliminary review on what you have so far?

chansuke · 2023-11-24T02:33:43Z

@stormqueen1990 Hi!Thank you for your comment.I've just started working on this issue and will finish soon. I will mention when it's ready.

stormqueen1990

Left some feedback! Please let me know if anything is unclear 😃

stormqueen1990 · 2023-11-28T00:50:03Z

kustomize/commands/version/version.go

+	bi, ok := debug.ReadBuildInfo()
+	if !ok {
+		return fmt.Errorf("unable to read build information")
+	}
+
+	var version string
+	for _, dep := range bi.Deps {
+		if dep.Path == "sigs.k8s.io/kustomize/kustomize" {
+			version = dep.Version
+			break
+		}
+	}


This change mirrors very closely the code in provenance.go:

kustomize/api/provenance/provenance.go

Lines 53 to 64 in bfb00ec

info, ok := debug.ReadBuildInfo()

if !ok {

return p

}

for _, setting := range info.Settings {

// For now, the git commit is the only information of interest.

// We could consider adding other info such as the commit date in the future.

if setting.Key == "vcs.revision" {

p.GitCommit = setting.Value

}

}

I think this code should be incorporated in that place, instead of added here.

@stormqueen1990 Thank you for your review!! I will update the function.

stormqueen1990 · 2023-11-28T00:50:52Z

kustomize/commands/version/version.go

+			fmt.Fprintln(o.Writer, version)
 		} else {
-			fmt.Fprintln(o.Writer, provenance.GetProvenance().Semver())
+			fmt.Fprintln(o.Writer, version)


question: this will cause the output to be the same regardless of whether the --short flag was passed or not. Is that intentional?

It wasn't intentional. I didn't read the code of provenance.go

stormqueen1990 · 2023-11-28T00:57:22Z

/ok-to-test

stormqueen1990

Hi @chansuke! 👋

Thank you very much for your contribution!

While this change looks good to me in principle, I'm afraid it doesn't address the issue. I tested it locally and kustomize version still outputs (devel) for me.

What I believe still needs to happen, in conjunction with this change, is an update to the Makefile, adding the -ldflags options to save the version in the built binary. Something similar to what currently exists here:

kustomize/releasing/create-release.sh

Lines 57 to 61 in bfb00ec

    
                 CGO_ENABLED=0 GOOS=$os GOARCH=$arch go build -o output/$binary_name -ldflags\ 
        
                   "-s -w\ 
        
                   -X sigs.k8s.io/kustomize/api/provenance.version=$version\ 
        
                   -X sigs.k8s.io/kustomize/api/provenance.gitCommit=$(git rev-parse HEAD)\ 
        
                   -X sigs.k8s.io/kustomize/api/provenance.buildDate=$build_date"\

One way to get the current working tree as a "development" version would be to use git describe --tags --always --dirty in the build process, in order to save that information in the version, something along the lines of:

go install -ldflags \
    "-X sigs.k8s.io/kustomize/api/provenance.buildDate=$(shell date -u +'%Y-%m-%dT%H:%M:%SZ') \
     -X sigs.k8s.io/kustomize/api/provenance.version=$(shell git describe --tags --always --dirty)" \
    .

Does this make sense?

stormqueen1990 · 2023-11-29T01:10:23Z

api/provenance/provenance.go

@@ -62,6 +62,13 @@ func GetProvenance() Provenance {
 			p.GitCommit = setting.Value
 		}
 	}
+
+	for _, dep := range info.Deps {
+		if dep.Path == "sigs.k8s.io/kustomize/kustomize" {


issue: this needs to be

Suggested change

if dep.Path == "sigs.k8s.io/kustomize/kustomize" {

if dep.Path == "sigs.k8s.io/kustomize/kustomize/v5" {

nit (non-blocking): it would be nice to have a constant for the module name with a comment explaining why we are looking for this specific dependency.

chansuke · 2023-11-29T16:18:39Z

@stormqueen1990 Yes, that's make sense and fixed Makefile too.

varshaprasad96

Agree to what @stormqueen1990 commented. Just added some more thoughts.

varshaprasad96 · 2023-12-05T14:25:39Z

api/provenance/provenance.go

@@ -62,6 +62,13 @@ func GetProvenance() Provenance {
 			p.GitCommit = setting.Value
 		}
 	}
+
+	for _, dep := range info.Deps {
+		if dep.Path == "sigs.k8s.io/kustomize/kustomize/v5" {


Better to also check if dep != nil, to avoid nil pointer error.

varshaprasad96 · 2023-12-05T14:34:46Z

api/provenance/provenance.go

+
+	for _, dep := range info.Deps {
+		if dep.Path == "sigs.k8s.io/kustomize/kustomize/v5" {
+			p.Version = dep.Version


If we are returning p.Version directly, we need to be aware of pseudo-versions too (golang/go#50603). We can sanitize it further to figure out the closest tag as done here (https://github.com/operator-framework/helm-operator-plugins/blob/d00d0092fe5c2a7776a678b7dcee4085cbccfe01/internal/version/version.go#L85). If we are comfortable with printing it as is, then that also seems good. It would be nice to add a comment in case we want to revisit it later.

@varshaprasad96 Thank you for the comment!! I will add some code to manage pseudo version

k8s-ci-robot · 2023-12-08T14:10:22Z

This PR has multiple commits, and the default merge method is: merge.
You can request commits to be squashed using the label: tide/merge-method-squash

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.

stormqueen1990 · 2023-12-11T03:31:47Z

api/provenance/provenance.go

+		if dep != nil {
+			continue
+		}
+		if dep.Path == "sigs.k8s.io/kustomize/kustomize/v5" {


I think this was meant to be:

Suggested change

if dep != nil {

continue

}

if dep.Path == "sigs.k8s.io/kustomize/kustomize/v5" {

if dep != nil && dep.Path == "sigs.k8s.io/kustomize/kustomize/v5" {

since we would like to check what's the Path of a given dep if and only if dep is not nil.

stormqueen1990 · 2023-12-13T03:50:03Z

Hi there, @chansuke! 👋

This is looking good. I see there's just a couple of lint issues being reported. Could you please address those?

You can also run the linter locally by doing make lint from the kustomize folder in this repository after you make the changes to ensure it's passing.

I'll keep an eye out for an update to leave my LGTM here.

Thanks for your contribution!

varshaprasad96

Thanks for the contribution :))

The changes look good to me, apart from a minor nit on adding tests. lgtm from my end, after the linter passes and @stormqueen1990's reviews are addressed.

varshaprasad96 · 2023-12-13T09:02:11Z

api/provenance/provenance.go

 	return p
 }

+func getMostRecentTag(m debug.Module) string {


The logic looks good! Could you add some unit tests to verify this piece of code? (If not in this PR, we can do it as a follow up too).

@varshaprasad96 Thank you for your review! I've add unit tests of GetMostRecentTag

stormqueen1990

/lgtm

/cc @varshaprasad96
for a second review

k8s-ci-robot · 2024-01-07T22:26:35Z

@stormqueen1990: GitHub didn't allow me to request PR reviews from the following users: a, second, review, for.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/lgtm

/cc @varshaprasad96 for a second review

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.

varshaprasad96

/lgtm
/approve

k8s-ci-robot · 2024-01-26T19:01:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chansuke, varshaprasad96

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:

~~OWNERS~~ [varshaprasad96]

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 added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 23, 2023

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 23, 2023

k8s-ci-robot requested review from natasha41575 and stormqueen1990 November 23, 2023 15:59

chansuke force-pushed the feat/show-version branch from f084ef6 to 2768527 Compare November 24, 2023 15:22

chansuke changed the title ~~WIP: Fix version tag management~~ Fix version tag management Nov 24, 2023

chansuke marked this pull request as ready for review November 24, 2023 15:23

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 24, 2023

k8s-ci-robot requested a review from yuwenma November 24, 2023 15:23

stormqueen1990 reviewed Nov 28, 2023

View reviewed changes

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 Nov 28, 2023

chansuke force-pushed the feat/show-version branch from 2768527 to d513839 Compare November 28, 2023 14:20

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 28, 2023

chansuke requested a review from stormqueen1990 November 28, 2023 14:20

stormqueen1990 reviewed Nov 29, 2023

View reviewed changes

chansuke force-pushed the feat/show-version branch from d513839 to 0d4e7e4 Compare November 29, 2023 16:17

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 29, 2023

chansuke requested a review from stormqueen1990 November 29, 2023 16:18

natasha41575 assigned stormqueen1990 and varshaprasad96 Nov 29, 2023

varshaprasad96 reviewed Dec 5, 2023

View reviewed changes

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 8, 2023

chansuke force-pushed the feat/show-version branch 2 times, most recently from b0465e3 to ff4a022 Compare December 8, 2023 15:15

chansuke requested a review from varshaprasad96 December 8, 2023 15:16

stormqueen1990 reviewed Dec 11, 2023

View reviewed changes

chansuke force-pushed the feat/show-version branch from ff4a022 to 9778337 Compare December 11, 2023 09:10

chansuke requested a review from stormqueen1990 December 11, 2023 09:11

varshaprasad96 reviewed Dec 13, 2023

View reviewed changes

chansuke force-pushed the feat/show-version branch from 9778337 to d7bca53 Compare December 13, 2023 15:50

Fix version tag management

cd88610

chansuke force-pushed the feat/show-version branch from c9c7253 to cd88610 Compare December 14, 2023 15:32

chansuke requested a review from varshaprasad96 December 14, 2023 15:34

stormqueen1990 reviewed Jan 7, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 7, 2024

varshaprasad96 approved these changes Jan 26, 2024

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 26, 2024

k8s-ci-robot merged commit abdcae8 into kubernetes-sigs:master Jan 26, 2024
9 checks passed

koba1t mentioned this pull request Apr 4, 2024

fix version subcommand is not working after release build #5644

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix version tag management #5463

Fix version tag management #5463

chansuke commented Nov 23, 2023

k8s-ci-robot commented Nov 23, 2023

stormqueen1990 commented Nov 23, 2023

chansuke commented Nov 24, 2023

stormqueen1990 left a comment

stormqueen1990 Nov 28, 2023

chansuke Nov 28, 2023

stormqueen1990 Nov 28, 2023

chansuke Nov 28, 2023

stormqueen1990 commented Nov 28, 2023

stormqueen1990 left a comment •

edited

stormqueen1990 Nov 29, 2023

chansuke commented Nov 29, 2023

varshaprasad96 left a comment

varshaprasad96 Dec 5, 2023

varshaprasad96 Dec 5, 2023

chansuke Dec 8, 2023

k8s-ci-robot commented Dec 8, 2023

stormqueen1990 Dec 11, 2023

stormqueen1990 commented Dec 13, 2023 •

edited

varshaprasad96 left a comment

varshaprasad96 Dec 13, 2023

chansuke Dec 14, 2023

stormqueen1990 left a comment •

edited

k8s-ci-robot commented Jan 7, 2024

varshaprasad96 left a comment

k8s-ci-robot commented Jan 26, 2024

	info, ok := debug.ReadBuildInfo()
	if !ok {
	return p
	}

	for _, setting := range info.Settings {
	// For now, the git commit is the only information of interest.
	// We could consider adding other info such as the commit date in the future.
	if setting.Key == "vcs.revision" {
	p.GitCommit = setting.Value
	}
	}

	CGO_ENABLED=0 GOOS=$os GOARCH=$arch go build -o output/$binary_name -ldflags\
	"-s -w\
	-X sigs.k8s.io/kustomize/api/provenance.version=$version\
	-X sigs.k8s.io/kustomize/api/provenance.gitCommit=$(git rev-parse HEAD)\
	-X sigs.k8s.io/kustomize/api/provenance.buildDate=$build_date"\

	if dep.Path == "sigs.k8s.io/kustomize/kustomize" {
	if dep.Path == "sigs.k8s.io/kustomize/kustomize/v5" {

Fix version tag management #5463

Fix version tag management #5463

Conversation

chansuke commented Nov 23, 2023

k8s-ci-robot commented Nov 23, 2023

stormqueen1990 commented Nov 23, 2023

chansuke commented Nov 24, 2023

stormqueen1990 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stormqueen1990 commented Nov 28, 2023

stormqueen1990 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chansuke commented Nov 29, 2023

varshaprasad96 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Dec 8, 2023

Choose a reason for hiding this comment

stormqueen1990 commented Dec 13, 2023 • edited

varshaprasad96 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stormqueen1990 left a comment • edited

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 7, 2024

varshaprasad96 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 26, 2024

stormqueen1990 left a comment •

edited

stormqueen1990 commented Dec 13, 2023 •

edited

stormqueen1990 left a comment •

edited