✨ Add version flag for operator-controller manager binary #802

yashoza19 · 2024-04-29T19:04:42Z

Description

resolves No commit/version command on operator-controller manager binary #328
this just adds git commit version for now. Do we want to also show the release version?

Reviewer Checklist

API Go Documentation
Tests: Unit Tests (and E2E Tests, if appropriate)
Comprehensive Commit Messages
Links to related GitHub Issue(s)

netlify · 2024-04-29T19:04:57Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`c5d750b`
🔍 Latest deploy log	https://app.netlify.com/sites/olmv1/deploys/663a32ba0898630008f55475
😎 Deploy Preview	https://deploy-preview-802--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

acornett21 · 2024-04-29T19:10:43Z

I'd personally like to see release info in the version cmd as well ie 1.0.0 and the GHA's to be updated to accommodate. Since it's easier to go from a release -> commit, then from a commit -> release when trying to trouble shoot or find the culprit of a bug. But is this not something the maintainers what, that is also fine.

varshaprasad96

@yashoza19 Thanks for the PR!

A couple of linter errors from the CI, and I agree with @acornett21 - having the release tag information mentioned is really helpful for debugging.

yashoza19 · 2024-04-30T04:52:36Z

Thanks for the comments @acornett21 and @varshaprasad96.

I have made the necessary changes to also show release tag information with git commit.

cmd/manager/main.go

perdasilva

lgtm - one small suggestion on the description for the help flag

acornett21 · 2024-04-30T14:30:24Z

I have made the necessary changes to also show release tag information with git commit.

@yashoza19 Does the GHA/Makefile need to be updated/enhanced accommodate this? Or do we get that for free somehow?

yashoza19 · 2024-04-30T14:48:36Z

@acornett21

Thank you for the email. Kindly, please note your case is in progress. Our team will reach out if any documents are needed from your end, as of today, confirming there are no actions pending from your end.

The MAKEFILE is updated in this PR. We can open another issue to update the GHA's if required.

codecov · 2024-05-03T15:51:16Z

Codecov Report

Attention: Patch coverage is 80.76923% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 71.15%. Comparing base (fadfe36) to head (628c960).
Report is 7 commits behind head on main.

Files	Patch %	Lines
cmd/manager/main.go	70.00%	2 Missing and 1 partial ⚠️
internal/version/version.go	87.50%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #802      +/-   ##
==========================================
+ Coverage   64.63%   71.15%   +6.51%     
==========================================
  Files          16       17       +1     
  Lines        1315     1300      -15     
==========================================
+ Hits          850      925      +75     
+ Misses        403      303     -100     
- Partials       62       72      +10

Flag	Coverage Δ
e2e	`42.84% <80.76%> (+0.94%)`	⬆️
unit	`63.59% <ø> (+5.87%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

varshaprasad96

The changes look good, just one nit!

varshaprasad96 · 2024-05-03T16:36:46Z

cmd/manager/main.go

 	ctrl.SetLogger(zap.New(zap.UseFlagOptions(&opts), zap.StacktraceLevel(zapcore.DPanicLevel)))
+	setupLog.Info("starting up the provisioner", "Git commit", version.String())


Nit: Its no longer a provisioner (as in rukpak), I'd make it a generic controller

Suggested change

setupLog.Info("starting up the provisioner", "Git commit", version.String())

setupLog.Info("starting up the controller", "Git commit", version.String())

joelanford · 2024-05-03T17:13:36Z

Makefile

+export GIT_REPO 		 ?= github.com/operator-framework/operator-controller
+export VERSION           ?= $(shell git describe --tags --always --abbrev=0)
+export VERSION_PATH 	 ?= ${GIT_REPO}/internal/version
+export GIT_COMMIT 		 ?= $(shell git rev-parse HEAD)


Don't use ?=, especially with shell commands. Because they run every time a variable is referenced. See the changes I made here: #813. I don't think these need to be overridable (do they?), so we can use use := instead of ?=.

Also, I see a mix of spaces and tabs. @m1kola had a good point when he reverted the vertical spacing I added in the above PR, so let's avoid that here as well.

joelanford · 2024-05-03T17:14:59Z

Makefile

@@ -207,10 +207,14 @@ CGO_ENABLED := 0
 endif
 export CGO_ENABLED

+export GIT_REPO 		 ?= github.com/operator-framework/operator-controller
+export VERSION           ?= $(shell git describe --tags --always --abbrev=0)


We already have a VERSION variable above. Delete this one. It is important to use --dirty so that a user knows if the binary was built from a modified git tree. And it is important not to use --abbrev=0 because non-tagged commits would look like they were tagged with that version.

joelanford · 2024-05-03T17:21:59Z

Makefile

 export GO_BUILD_GCFLAGS := all=-trimpath=$(PWD)
 export GO_BUILD_FLAGS :=
+export GO_BUILD_LDFLAGS  := -s -w -X '$(VERSION_PATH).version=$(VERSION)' -X '$(VERSION_PATH).gitCommit=$(GIT_COMMIT)'


Nit to avoid long lines (note the extra empty line at the end so that we can put \ at the end of each line consistently).

Suggested change

export GO_BUILD_LDFLAGS := -s -w -X '$(VERSION_PATH).version=$(VERSION)' -X '$(VERSION_PATH).gitCommit=$(GIT_COMMIT)'

export GO_BUILD_LDFLAGS := -s -w \

-X '$(VERSION_PATH).version=$(VERSION)' \

-X '$(VERSION_PATH).gitCommit=$(GIT_COMMIT)' \

joelanford · 2024-05-03T17:27:29Z

Makefile

+export GIT_REPO 		 ?= github.com/operator-framework/operator-controller
+export VERSION           ?= $(shell git describe --tags --always --abbrev=0)
+export VERSION_PATH 	 ?= ${GIT_REPO}/internal/version
+export GIT_COMMIT 		 ?= $(shell git rev-parse HEAD)


Looks like debug.BuildInfo contains the git commit with key vcs.revision. Could we use that instead of injecting it?

https://pkg.go.dev/runtime/debug#BuildSetting

yashoza19 · 2024-05-03T18:08:39Z

@joelanford and @varshaprasad96, Thanks for the review. I have made the suggested changes, PTAL whenever you get a chance. Thanks!

internal/version/version.go

joelanford · 2024-05-06T19:13:40Z

Makefile

@@ -207,10 +207,13 @@ CGO_ENABLED := 0
 endif
 export CGO_ENABLED

+export GIT_REPO := github.com/operator-framework/operator-controller


Oh one more think I noticed: This technically needs to be to go module name in order to be used within the $(VERSION_PATH) variable. So in that case, can we be explicit (and more copy/paste friendly) with this:

Suggested change

export GIT_REPO := github.com/operator-framework/operator-controller

export GIT_REPO := $(shell go list -m)

everettraven · 2024-05-07T13:46:55Z

cmd/manager/main.go

 	ctrl.SetLogger(zap.New(zap.UseFlagOptions(&opts), zap.StacktraceLevel(zapcore.DPanicLevel)))
+	setupLog.Info("starting up the controller", "Git commit", version.String())


Nit: This isn't just the git commit so that might be a bit misleading in the logs. Maybe something like:

Suggested change

setupLog.Info("starting up the controller", "Git commit", version.String())

setupLog.Info("starting up the controller", "version info", version.String())

Good catch. fixed it.

everettraven

One minor nit. Otherwise looks good to me, thanks @yashoza19 !

Signed-off-by: yashoza19 <yoza@redhat.com>

yashoza19 requested a review from a team as a code owner April 29, 2024 19:04

yashoza19 changed the title ~~add version flag for operator-controller manager binary~~ ✨ Add version flag for operator-controller manager binary Apr 29, 2024

varshaprasad96 reviewed Apr 29, 2024

View reviewed changes

yashoza19 force-pushed the issue328 branch from 76b03b5 to ad90ec9 Compare April 30, 2024 04:48

yashoza19 requested a review from varshaprasad96 April 30, 2024 04:59

perdasilva reviewed Apr 30, 2024

View reviewed changes

cmd/manager/main.go Outdated Show resolved Hide resolved

perdasilva previously approved these changes Apr 30, 2024

View reviewed changes

yashoza19 dismissed perdasilva’s stale review via 4856ada April 30, 2024 13:11

yashoza19 force-pushed the issue328 branch from ad90ec9 to 4856ada Compare April 30, 2024 13:11

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 2, 2024

yashoza19 force-pushed the issue328 branch from 4856ada to eed1dd0 Compare May 2, 2024 20:21

openshift-merge-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 May 2, 2024

yashoza19 force-pushed the issue328 branch from eed1dd0 to 973a7f0 Compare May 3, 2024 13:49

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 3, 2024

varshaprasad96 previously approved these changes May 3, 2024

View reviewed changes

joelanford reviewed May 3, 2024

View reviewed changes

yashoza19 dismissed varshaprasad96’s stale review via 6d85720 May 3, 2024 18:06

yashoza19 force-pushed the issue328 branch from 973a7f0 to 6d85720 Compare May 3, 2024 18:06

everettraven reviewed May 6, 2024

View reviewed changes

internal/version/version.go Show resolved Hide resolved

joelanford reviewed May 6, 2024

View reviewed changes

yashoza19 force-pushed the issue328 branch 3 times, most recently from 628c960 to b3d43cc Compare May 7, 2024 13:39

everettraven reviewed May 7, 2024

View reviewed changes

everettraven previously approved these changes May 7, 2024

View reviewed changes

yashoza19 dismissed everettraven’s stale review via bcca8d1 May 7, 2024 13:50

yashoza19 force-pushed the issue328 branch 3 times, most recently from d9e2ecf to 7423897 Compare May 7, 2024 13:52

everettraven previously approved these changes May 7, 2024

View reviewed changes

add version flag for operator-controller manager binary

c5d750b

Signed-off-by: yashoza19 <yoza@redhat.com>

yashoza19 dismissed everettraven’s stale review via c5d750b May 7, 2024 13:55

yashoza19 force-pushed the issue328 branch from 7423897 to c5d750b Compare May 7, 2024 13:55

everettraven approved these changes May 7, 2024

View reviewed changes

joelanford added this pull request to the merge queue May 7, 2024

Merged via the queue into operator-framework:main with commit 6d73b73 May 7, 2024
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add version flag for operator-controller manager binary #802

✨ Add version flag for operator-controller manager binary #802

yashoza19 commented Apr 29, 2024

netlify bot commented Apr 29, 2024 •

edited

Loading

acornett21 commented Apr 29, 2024

varshaprasad96 left a comment

yashoza19 commented Apr 30, 2024

perdasilva left a comment

acornett21 commented Apr 30, 2024 •

edited

Loading

yashoza19 commented Apr 30, 2024

codecov bot commented May 3, 2024 •

edited

Loading

varshaprasad96 left a comment

varshaprasad96 May 3, 2024

joelanford May 3, 2024 •

edited

Loading

joelanford May 3, 2024 •

edited

Loading

joelanford May 3, 2024 •

edited

Loading

joelanford May 3, 2024 •

edited

Loading

yashoza19 commented May 3, 2024

joelanford May 6, 2024

everettraven May 7, 2024

yashoza19 May 7, 2024

everettraven left a comment

		ctrl.SetLogger(zap.New(zap.UseFlagOptions(&opts), zap.StacktraceLevel(zapcore.DPanicLevel)))
		setupLog.Info("starting up the provisioner", "Git commit", version.String())

	setupLog.Info("starting up the provisioner", "Git commit", version.String())
	setupLog.Info("starting up the controller", "Git commit", version.String())

-export GO_BUILD_LDFLAGS  := -s -w -X '$(VERSION_PATH).version=$(VERSION)' -X '$(VERSION_PATH).gitCommit=$(GIT_COMMIT)'
+export GO_BUILD_LDFLAGS  := -s -w \
+    -X '$(VERSION_PATH).version=$(VERSION)' \
+    -X '$(VERSION_PATH).gitCommit=$(GIT_COMMIT)' \

	export GIT_REPO := github.com/operator-framework/operator-controller
	export GIT_REPO := $(shell go list -m)

✨ Add version flag for operator-controller manager binary #802

✨ Add version flag for operator-controller manager binary #802

Conversation

yashoza19 commented Apr 29, 2024

Description

Reviewer Checklist

netlify bot commented Apr 29, 2024 • edited Loading

✅ Deploy Preview for olmv1 ready!

acornett21 commented Apr 29, 2024

varshaprasad96 left a comment

Choose a reason for hiding this comment

yashoza19 commented Apr 30, 2024

perdasilva left a comment

Choose a reason for hiding this comment

acornett21 commented Apr 30, 2024 • edited Loading

yashoza19 commented Apr 30, 2024

codecov bot commented May 3, 2024 • edited Loading

Codecov Report

varshaprasad96 left a comment

Choose a reason for hiding this comment

varshaprasad96 May 3, 2024

Choose a reason for hiding this comment

joelanford May 3, 2024 • edited Loading

Choose a reason for hiding this comment

joelanford May 3, 2024 • edited Loading

Choose a reason for hiding this comment

joelanford May 3, 2024 • edited Loading

Choose a reason for hiding this comment

joelanford May 3, 2024 • edited Loading

Choose a reason for hiding this comment

yashoza19 commented May 3, 2024

joelanford May 6, 2024

Choose a reason for hiding this comment

everettraven May 7, 2024

Choose a reason for hiding this comment

yashoza19 May 7, 2024

Choose a reason for hiding this comment

everettraven left a comment

Choose a reason for hiding this comment

netlify bot commented Apr 29, 2024 •

edited

Loading

acornett21 commented Apr 30, 2024 •

edited

Loading

codecov bot commented May 3, 2024 •

edited

Loading

joelanford May 3, 2024 •

edited

Loading

joelanford May 3, 2024 •

edited

Loading

joelanford May 3, 2024 •

edited

Loading

joelanford May 3, 2024 •

edited

Loading