add version flavour, print version command #229
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #229 +/- ##
==========================================
- Coverage 32.35% 32.09% -0.27%
==========================================
Files 75 75
Lines 9710 9713 +3
==========================================
- Hits 3142 3117 -25
- Misses 5930 5957 +27
- Partials 638 639 +1 ☔ View full report in Codecov by Sentry. |
Makefile
Outdated
| @@ -12,8 +12,10 @@ BINARY=keymaster | |||
| # These are the values we want to pass for Version and BuildTime | |||
| VERSION=1.15.3 | |||
| DEFAULT_HOST?= | |||
| DEFAULT_LDFLAGS=-X main.Version=${VERSION} | |||
| CLIENT_LDFLAGS=${DEFAULT_LDFLAGS} -X main.defaultHost=${DEFAULT_HOST} | |||
| VERSION_FLAVOUR?=cf | |||
There was a problem hiding this comment.
I don't think the "upstream" version should have a flavour. It's the default. This should be the empty string.
There was a problem hiding this comment.
can it be main / upstream? Otherwise it prints "1.15.3-" as the version without further changes.
There was a problem hiding this comment.
This can be solved by having the flavour string that's injected contain a leading -.
There was a problem hiding this comment.
Why so many changes? I would have done this with a new conditional on the makefile and a new variable:
PRINTVERSION=version
ifeq ($(FLAVOR),)
PRINTVERTION = ${VERSION}-${FLAVOR}
And then use the printversion to the build instead of version.
Also I dont like using the useragent string for something else.
There was a problem hiding this comment.
Thats doable as well :) so maybe lets close this PR and when you have it done I will merge it to the PS fork.
There was a problem hiding this comment.
Also, I still believe that useragent is better for debugging than the plain version as it contains also OS and Arch. If somebody pastes this in the report, it saves time / roundtrips for questions which OS do you use.
ph4r05
force-pushed
the
pr
branch
2 times, most recently
from
6180709
to
6c3868f
Compare
|
I've also added Makefile as a single source of truth for version. keymaster.spec is then generated in the makefile with sed. It is technically not needed for Windows but I am doing it anyway - tested on our windows builders. |
| @@ -496,6 +497,10 @@ func main() { | |||
| flag.Usage = Usage | |||
| flag.Parse() | |||
| logger := cmdlogger.New() | |||
| if *printVersion { | |||
| fmt.Println(Version) | |||
There was a problem hiding this comment.
nit: use the logger instead of directly using fmt.
There was a problem hiding this comment.
the thing is that logger prints it out to stderr, it made more sense to me to print it to stdout
cmd/keymaster/main_test.go
Outdated
| main() | ||
| close(done) |
There was a problem hiding this comment.
This is not really a good test as you are not even checking for the output of the call or its side-effects. Testing main IS complicated. And I think the way that is needed is to make a new : mainWithError(logger log.Logger) error. That basically does everything except that it no longer calls fatal, but returns and error. And the main will just call this mainWithError with the initialized logger. Now we can start testing mainWithError. And check for the return types and the sideEffects.
Not for today (unless you want too).
There was a problem hiding this comment.
@cviecco I added the test, pls check
- make makefile single source of truth for version - trigger the flow in the tests
* add flavour, version command, fix version source (Cloud-Foundations#229) - make makefile single source of truth for version - trigger the flow in the tests * minor tests enhancements (Cloud-Foundations#232) --------- Co-authored-by: Dušan Klinec <[email protected]> Co-authored-by: cviecco <[email protected]>
* add flavour, version command, fix version source (Cloud-Foundations#229) - make makefile single source of truth for version - trigger the flow in the tests * minor tests enhancements (Cloud-Foundations#232) * Docker cleanup (Cloud-Foundations#233) * Removed unnecessary `start.sh` * Updated Dockerfile to newer OS * Cleaned up Dockerfile dirty hack for RSA keys Co-authored-by: Espinoza, Erik <[email protected]> * enable to specify agent connection to insert cert to (Cloud-Foundations#231) * enable to specify agent connection to insert cert to * add api * bump version --------- Co-authored-by: Dušan Klinec <[email protected]> --------- Co-authored-by: Dušan Klinec <[email protected]> Co-authored-by: cviecco <[email protected]> Co-authored-by: Erik Espinoza <[email protected]> Co-authored-by: Espinoza, Erik <[email protected]>
Changes from PS fork