added code to display version as commit hash

[ehearneRedHat]

What:

• Added code to display version as commit hash when running ./kuadrantctl version if Git is installed. Otherwise, it will show version v0.0.0 .

Verify:

• Run go build followed by ./kuadrantctl version . The output should be something like:

{"level":"info","ts":"2024-05-22T12:58:34+01:00","msg":"kuadrantctl version: dev - 4d98872"}

If Git is not installed, the output should look like:

{"level":"info","ts":"2024-05-22T12:58:34+01:00","msg":"kuadrantctl version: dev - v0.0.0"}

[R-Lawton]

Im apprehensive about running git command as part of the version functionality, Im thinking we could make this simpler and have less requests to GH if we add it as a step in the install make target so it just changes the version var and that can be used like it is now for latest. The only thing is we are currently installing via go install and instead we want to use make install which uses go install but would have the extra version step so a update to the readme would be needed. wdyt?

[ehearneRedHat]

Im apprehensive about running git command as part of the version functionality

That's fair enough - adding an external dependency can present issue.

Im thinking we could make this simpler and have less requests to GH

The proposed solution now, is to use the git command and source the commit hash locally. No requests to GitHub required.

add it as a step in the install make target so it just changes the version var and that can be used like it is now for latest

Make target isn't a bad idea - however, git will still need to be used, so git command would be called outside of GoLang rather than within GoLang.

we are currently installing via go install and instead we want to use make install which uses go install

Okay, makes sense. Having things done in a single location provides better simplicity and improve what a user expects. I will research how to do my proposed solution via a make target instead.

have the extra version step so a update to the readme would be needed

Sure, so to update the README to reflect this new feature, right? That makes sense. Within my current PR, I will go ahead and implement a solution with your suggestions in mind. It will remain as a WIP until this is done, and I can ping you along the way to ensure this solution suits. :)

Thanks for your suggestions, @R-Lawton !

[ehearneRedHat]

@R-Lawton I have implemented your suggestions - do they align with what you suggested ? :)

[ehearneRedHat]

@adam-cattermole is this what you were suggesting ?

[ehearneRedHat]

@R-Lawton let me know if this what you were looking for :)

If so, I'll go ahead and set the PR as Ready for review.

[R-Lawton]

LGTM make target works verified the right version shows

[ehearneRedHat]

Thanks @R-Lawton for the approval. I remember @azgabur wanted to also review this PR, so I will request review now. :)

[azgabur]

Right now there are now 3 scenarios with different version values:

1. go build creates binary which prints empty version
2. make install without git configured creates binary with version "dev -"
3. make install with git configured creates binary with correct version with commit hash

I would somehow merge the 1. and 2., maybe setting a default value in version/version.go and not in Makefile?

[ehearneRedHat]

@azgabur added requested changes - please review when you have the chance. :)

[eguzki]

I would like to propose a different approach. I am trying to align the "versioning system" with other kuadrant components like limitador

❯ docker run --rm -t quay.io/kuadrant/limitador:v1.5.0 limitador-server --version
Limitador Server v1.5.0 (efe9ac87) [] release

Or WIP, the kuadrant wasm-shim Kuadrant/wasm-shim#53 which would generate something like

Kuadrant wasm module v0.5.0-dev (7ea94e90) ["+with-serde"] release root-context #1: VM started

The proposed format is

"msg":"kuadrantctl version: <version> (<git-hash>)"

Rules for <version>

main branch no longer has v0.0.0. It has v<next-unreleased-version>-dev. So if the current release is v0.2.3, version.go file would be like this

package version

var (
	Version = "v0.2.4-dev"
)

thatdev suffix means it is still under development.

The v0.2.4 release process would have an extra step to bump the version to Version = "v0.2.4" and main would be bumped to Version = "v0.2.5-dev"

Rules for <git-hash>

The build process first tries

GIT_SHA = `git rev-parse HEAD`

if it suceeded, it tries to check for "dirty" files (not committed files used in the build)

GIT_DIRTY = `git diff --stat`

So GIT_HASH is either

GIT_HASH = $(GIT_SHA)

or

GIT_HASH = $(GIT_SHA)-dirty

If it fails (because there is no git in the build process), it tries to read env var GITHUB_SHA, so

GIT_HASH = $(GITHUB_SHA)

It fallsback to unknown when even the GITHUB_SHA env var does not exist.

WDYT?

[ehearneRedHat]

Hey @eguzki,

Thanks for your suggestion! I will work on the suggestion and ping you for review when it is ready. Thanks!

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 58.66%. Comparing base (7296e1b) to head (a44d61c).
Report is 45 commits behind head on main.

Files	Patch %	Lines
cmd/version.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##            main      #85       +/-   ##
==========================================
+ Coverage   0.38%   58.66%   +58.27%     
==========================================
  Files         17       17               
  Lines        783      612      -171     
==========================================
+ Hits           3      359      +356     
+ Misses       780      181      -599     
- Partials       0       72       +72     

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

[ehearneRedHat]

No worries, @eguzki. I will probably continue with this PR for now (if there are any changes that are not surrounded by this issue I will put those in a separate PR) . :)

[ehearneRedHat]

Hey @eguzki I have amended your suggestions - let me know if it looks good to you. :)

[ehearneRedHat]

@eguzki I have amended your suggestions - let me know if this suits :)

[eguzki]

Good job 🎖️

It is working as defined. I tried with "clean" git, "dirty" git and without git and GITHUB_SHA env set and worked as expected.

Before approval, some comments for final polishing

[ehearneRedHat]

Thanks @eguzki for being patient with me as I work through the suggestions - they have been amended. :) Feel free to review.

[eguzki]

Thanks @eguzki for being patient with me as I work through the suggestions - they have been amended. :) Feel free to review.

no need to be grateful, you are more than welcome. Sorry for being (sometimes) too nitpicking.

[eguzki]

This is almost done.

I realized that we might have an issue with the release GH workflow. On every github release, this workflow is triggered .github/workflows/release.yaml

    steps:
    - uses: actions/checkout@v3
    - uses: wangyoucao577/go-release-action@v1
      with:
        github_token: ${{ secrets.GITHUB_TOKEN }}
        goos: ${{ matrix.goos }}
        goarch: ${{ matrix.goarch }}
        goversion: 1.21.4

Which basically does go install, thus, version.GitHash would not be filled.

looking at the doc https://github.com/wangyoucao577/go-release-action it seems that the action allows "rich parameter support for go build".

It would be nice to have the "Git_hash" filled for release binaries (actually those are the candidates to be used out there).

You can try creating "fake" releases to trigger the workflow. Just set them as "pre-release" and give them clarifying names like: "v0.2.4-alpha-1", "v0.2.4-alpha-2", ...

[ehearneRedHat]

It would be nice to have the "Git_hash" filled for release binaries (actually those are the candidates to be used out there).

I can probably do this in a separate PR if that suits? Either way, ready for review, @eguzki !

[eguzki]

LGTM 🏅

[eguzki]

Last, but not least, I would bring this version with commit hash implementation to kuadrant operators (all of them).

[ehearneRedHat]

@eguzki that's quite the task you suggested! No worries, I'll go ahead and add that as an issue to all operators. I'll go ahead and add the issue to add the GitHash to the GitHub Actions Workflow . Thanks for the approval! Merging now...