Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MYST-563 Compile version info executable #268

Merged
merged 14 commits into from Jun 26, 2018

Conversation

Projects
None yet
3 participants
@Waldz
Copy link
Member

commented Jun 18, 2018

No description provided.

@Waldz Waldz requested review from tadovas, interro and zolia Jun 18, 2018

@Waldz Waldz requested a review from donce as a code owner Jun 18, 2018

@Waldz Waldz changed the title MYST-563 compile version info executable MYST-563 Compile version info executable Jun 18, 2018

@@ -54,8 +54,8 @@ const statusConnected = "Connected"

// Run runs CLI interface synchronously, in the same thread while blocking it
func (c *Command) Run() (err error) {
startupLicense := license.GetStartupLicense("type 'license warranty'", "type 'license conditions'")
fmt.Print(startupLicense + "\n")
printHeader()

This comment has been minimized.

Copy link
@tadovas

tadovas Jun 19, 2018

Member

Maybe printLicence() ?

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 22, 2018

Author Member

It's text the program says hello with (version, licence, etc.)

This comment has been minimized.

Copy link
@tadovas

tadovas Jun 22, 2018

Member

👍
But still I would like to have info like build, version etc also printed to logs - in that case we can have all information in single place and can ignore direct stdout prints

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 25, 2018

Author Member

Added version to logs also

func printLicense() {
startupLicense := license.GetStartupLicense(
func printHeader() {
fmt.Println("Program: Mysterium client")

This comment has been minimized.

Copy link
@tadovas

tadovas Jun 19, 2018

Member

Why we need to use stdout directly? the point of using logging framework was to print everything to log and leave streams( file, stdout) for logging configuration

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 22, 2018

Author Member

Logs is a time-series data with debug information.
Here is text here program says hello to user.

@@ -27,7 +27,7 @@ import (
// ApplicationStopper stops application and performs required cleanup tasks
type ApplicationStopper func()

// StopOnInterrupts invokes given stopper on SIGTERM and SIGHUP interrupts with additional wait condition
// StopOnInterruptsConditional invokes given stopper on SIGTERM and SIGHUP interrupts with additional wait condition

This comment has been minimized.

Copy link
@tadovas

tadovas Jun 19, 2018

Member

Why this new naming is better? What does "conditional" mean in this context?

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 22, 2018

Author Member

I just made docblock consistent with code

This comment has been minimized.

Copy link
@tadovas

tadovas Jun 22, 2018

Member

Oh I see. By the way this function will be removed anyway in another PR :)

@@ -51,6 +50,7 @@ func TestHealthCheckReturnsExpectedJSONObject(t *testing.T) {
"uptime" : "1m0s",
"process" : 1,
"version" : {
"version": "`+params.VersionAsString()+`",

This comment has been minimized.

Copy link
@tadovas

tadovas Jun 19, 2018

Member

version inside version ? maybe simply "id", "name", "text", "string" ?

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 25, 2018

Author Member

Improved DTO structure


// Package params contains build information of executable usually provided by
// automated build systems like Travis. Default values are populated if not overriden by build system
package params

This comment has been minimized.

Copy link
@tadovas

tadovas Jun 19, 2018

Member

This package name is misleading, maybe "metadata"? Because it contains build info, version info, licenses etc etc. What kind of parameters are these ? Or what's bad with separate package for everything like "build" , "legal", "version" ?

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 25, 2018

Author Member

Yes, it's global system params, which may be needed for any package in system

@@ -40,7 +40,6 @@ fi
MESSAGES_ERROR=`golint --set_exit_status --min_confidence=1 ${ARGUMENTS} 2>/dev/null`
MESSAGES_RECONFIGURE=()

check_uncleaned_package "github.com/mysterium/node/cmd" 3

This comment has been minimized.

Copy link
@donce

donce Jun 19, 2018

Contributor

Why? ;(

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 25, 2018

Author Member

Because is not uncleaned package anymore

fmt.Println("Program: Mysterium client")
fmt.Println("Version: ", params.VersionAsString())
fmt.Println("Build info: ", params.BuildAsString())
fmt.Println("Copyright: \n", params.GetStartupLicense(

This comment has been minimized.

Copy link
@donce

donce Jun 19, 2018

Contributor

no space needed before newline

@@ -15,8 +15,22 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

package license
package params

This comment has been minimized.

Copy link
@donce

donce Jun 19, 2018

Contributor

Mistake in filename - license.go, not licence.go

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 25, 2018

Author Member

fixed

@@ -1,5 +1,5 @@
/*
* Copyright (C) 2017 The "MysteriumNetwork/node" Authors.
* Copyright (C) 2018 The "MysteriumNetwork/node" Authors.

This comment has been minimized.

Copy link
@donce

donce Jun 19, 2018

Contributor

We shouldn't update copyright years - it should be year of creation, not year of last modified date.

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 25, 2018

Author Member

It's a new file

Version buildInfo `json:"version"`
Uptime string `json:"uptime"`
Process int `json:"process"`
Version versionData `json:"version"`

This comment has been minimized.

Copy link
@donce

donce Jun 19, 2018

Contributor

Version = version + commit + branch + buildNumber
Naming is very strange here, isn't commit + branch + buildNumber just a buildInfo?

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 25, 2018

Author Member

Improved

}
}

func (hce *healthCheckEndpoint) HealthCheck(writer http.ResponseWriter, request *http.Request, params httprouter.Params) {
func (hce *healthCheckEndpoint) HealthCheck(writer http.ResponseWriter, request *http.Request, routeParams httprouter.Params) {

This comment has been minimized.

Copy link
@donce

donce Jun 19, 2018

Contributor

You meant to rename to routerParams?

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 25, 2018

Author Member

Yes, it conflicted with package name

@Waldz Waldz force-pushed the feature/MYST-563-compile-version branch from f1d4a8b to ab07aca Jun 22, 2018

Waldz added some commits Jun 13, 2018

Create params package
Signed-off-by: Waldz <valdas@mysterium.network>
Move licence to params package
Signed-off-by: Waldz <valdas@mysterium.network>
Rename version to buildInfo
Signed-off-by: Waldz <valdas@mysterium.network>
Compile semantic version into code
Signed-off-by: Waldz <valdas@mysterium.network>
Return version via healthcheck endpoint
Signed-off-by: Waldz <valdas@mysterium.network>
Print version on program startup
Signed-off-by: Waldz <valdas@mysterium.network>
Interactive subcommands to checks version
Signed-off-by: Waldz <valdas@mysterium.network>
Rename version.version to version.id
Signed-off-by: Waldz <valdas@mysterium.network>
Fix mistype in license
Signed-off-by: Waldz <valdas@mysterium.network>
Improve version DTO in Tequilapi
Signed-off-by: Waldz <valdas@mysterium.network>
Remove CMD relations to standard output
Signed-off-by: Waldz <valdas@mysterium.network>
(cherry picked from commit a4a1f7907d5734e593a178d4810a95f29aa521d4)
Add version to program logs
Signed-off-by: Waldz <valdas@mysterium.network>

@Waldz Waldz force-pushed the feature/MYST-563-compile-version branch from ab07aca to 99b1507 Jun 25, 2018

@tadovas
Copy link
Member

left a comment

LGTM

Rename package: params -> metadata
Signed-off-by: Waldz <valdas@mysterium.network>

@Waldz Waldz force-pushed the feature/MYST-563-compile-version branch from 813f1fd to 3665c83 Jun 25, 2018

@tadovas
Copy link
Member

left a comment

LGTM

@@ -88,6 +88,16 @@ func (c *Command) Run() (err error) {
}
}

func printVersion() {

This comment has been minimized.

Copy link
@donce

donce Jun 25, 2018

Contributor

Name is misleading - it doesn't print just version, it prints many stuff. printStartupInfo() ?

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 26, 2018

Author Member

Improved naming

fmt.Println("Copyright:", metadata.GetStartupLicense(
"run program with '-warranty' option",
"run program with '-conditions' option",
))

This comment has been minimized.

Copy link
@donce

donce Jun 25, 2018

Contributor

Hmm, do we really need to show copyright when running with --version flag? Maybe we should show current MYST value as well? :D

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 26, 2018

Author Member

It's ok, lots of programs does this: print versions and data related to version (build info, runtime info, copyright, modules, etc.)

@@ -1,5 +1,5 @@
/*
* Copyright (C) 2017 The "MysteriumNetwork/node" Authors.
* Copyright (C) 2018 The "MysteriumNetwork/node" Authors.

This comment has been minimized.

Copy link
@donce

donce Jun 25, 2018

Contributor

No need to update years.

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 26, 2018

Author Member

It's not a change, it's new file

Improve version texts
Signed-off-by: Waldz <valdas@mysterium.network>

@Waldz Waldz force-pushed the feature/MYST-563-compile-version branch from 9135b0c to 36f2caa Jun 26, 2018

@donce

donce approved these changes Jun 26, 2018

@tadovas
Copy link
Member

left a comment

LGTM

@Waldz Waldz merged commit 2c0f6be into master Jun 26, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@Waldz Waldz deleted the feature/MYST-563-compile-version branch Jun 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.