Kuadrant / kuadrantctl

Kuadrant configuration command line utility
Apache License 2.0
6 stars 13 forks source link

added code to display version as commit hash #85

Closed ehearneRedHat closed 3 months ago

ehearneRedHat commented 4 months ago

What:

Verify:

{"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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

ehearneRedHat commented 4 months ago

@adam-cattermole is this what you were suggesting ?

ehearneRedHat commented 4 months ago

@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.

ehearneRedHat commented 4 months ago

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

ehearneRedHat commented 4 months ago

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

eguzki commented 4 months ago

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 https://github.com/Kuadrant/wasm-shim/pull/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 commented 3 months ago

Hey @eguzki,

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

codecov-commenter commented 3 months ago

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 :warning:
Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

eguzki commented 3 months ago

@ehearneRedHat up to you. This PR has already been approved by two people. So, feel free to merge it and have a following up PR with the proposed changes.

ehearneRedHat commented 3 months ago

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 commented 3 months ago

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

ehearneRedHat commented 3 months ago

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

ehearneRedHat commented 3 months ago

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

eguzki commented 3 months ago

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.

ehearneRedHat commented 3 months ago

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 commented 3 months ago

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

ehearneRedHat commented 3 months ago

@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...