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

build info in struct /healthcheck #192

Merged
merged 2 commits into from Mar 5, 2018

Conversation

Projects
None yet
3 participants
@shroomist
Copy link
Contributor

commented Mar 3, 2018

No description provided.

build info returned in struct
Signed-off-by: Andrej Novikov <jazzaiman@gmail.com>

@shroomist shroomist requested review from tadovas and donce Mar 4, 2018

@@ -4,6 +4,12 @@ package version

import "fmt"

type Info struct {

This comment has been minimized.

Copy link
@donce

donce Mar 4, 2018

Contributor

Missing doc.

@@ -17,3 +23,11 @@ var BuildNumber = "dev-build"
func AsString() string {
return fmt.Sprintf("Branch: %s. Build id: %s. Commit: %s", GitBranch, BuildNumber, GitCommit)
}

func GetInfo() *Info {

This comment has been minimized.

Copy link
@donce

donce Mar 4, 2018

Contributor

Missing doc.

}

type healthCheckEndpoint struct {
startTime time.Time
currentTimeFunc func() time.Time
processNumber int
buildVersion string
buildVersion *version.Info

This comment has been minimized.

Copy link
@donce

donce Mar 4, 2018

Contributor

maybe versionInfo is more accurate name?

type buildInfo struct {
Commit string `json:"commit"`
Branch string `json:"branch"`
BuildNumber string `json:"buildNumber"`

This comment has been minimized.

Copy link
@donce

donce Mar 4, 2018

Contributor

buildInfo is identical to version.Info, except json names. Is this intentional to make this module less coupled to version package..?

This comment has been minimized.

Copy link
@shroomist

shroomist Mar 5, 2018

Author Contributor

yes

@@ -17,3 +23,11 @@ var BuildNumber = "dev-build"
func AsString() string {
return fmt.Sprintf("Branch: %s. Build id: %s. Commit: %s", GitBranch, BuildNumber, GitCommit)
}

func GetInfo() *Info {

This comment has been minimized.

Copy link
@donce

donce Mar 4, 2018

Contributor

Since we're exposing this method to get version info, can GitCommit, GitBranch and BuildNumber be private? Or do they have to be public because we set them in bash script?

This comment has been minimized.

Copy link
@tadovas

tadovas Mar 5, 2018

Member

The logic should be very simple. Version package provides a few exported variables and a function which concats them to string. Why do we need separate GetInfo function with variables put in structure? Just define structure where you need it - i.e. in healthcheck endpoint


TRAVIS_COMMIT="$(git rev-parse --short HEAD 2> /dev/null | sed "s/\(.*\)/\1/")"
TRAVIS_BRANCH="$(git symbolic-ref --short -q HEAD)"

This comment has been minimized.

Copy link
@tadovas

tadovas Mar 5, 2018

Member

Travis CI has it's own target branch exposing logic. Why are you are overriding this?
At least put both variables in conditional logc if its not set - set to default value

@@ -17,3 +23,11 @@ var BuildNumber = "dev-build"
func AsString() string {
return fmt.Sprintf("Branch: %s. Build id: %s. Commit: %s", GitBranch, BuildNumber, GitCommit)
}

func GetInfo() *Info {

This comment has been minimized.

Copy link
@tadovas

tadovas Mar 5, 2018

Member

The logic should be very simple. Version package provides a few exported variables and a function which concats them to string. Why do we need separate GetInfo function with variables put in structure? Just define structure where you need it - i.e. in healthcheck endpoint

@donce
Copy link
Contributor

left a comment

Same issues persist.

@shroomist shroomist force-pushed the feature/MYST-320-versionEndPoint branch from 55c9731 to dbb3cb1 Mar 5, 2018

@tadovas
Copy link
Member

left a comment

LGTM

}

/*
HealthCheckEndpointFactory creates a structure with single HealthCheck method for healthcheck serving as http,
currentTimeFunc is injected for easier testing
*/
func HealthCheckEndpointFactory(currentTimeFunc func() time.Time, procID func() int, buildVersion string) *healthCheckEndpoint {
func HealthCheckEndpointFactory(currentTimeFunc func() time.Time, procID func() int, buildVersion *version.Info) *healthCheckEndpoint {

This comment has been minimized.

Copy link
@donce

donce Mar 5, 2018

Contributor

buildVersion -> versionInfo


// BuildNumber comes from TRAVIS_JOB_NUMBER env variable
var BuildNumber = "dev-build"
var buildNumber = "dev-build"

This comment has been minimized.

Copy link
@donce

donce Mar 5, 2018

Contributor

comments of GitCommit, GitBranch and BuildNumber should be lowercased as well.

review fixes
Signed-off-by: Andrej Novikov <jazzaiman@gmail.com>

@shroomist shroomist force-pushed the feature/MYST-320-versionEndPoint branch from dbb3cb1 to e7a9376 Mar 5, 2018

@donce

donce approved these changes Mar 5, 2018

@shroomist shroomist merged commit 5abcaa5 into master Mar 5, 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

@donce donce deleted the feature/MYST-320-versionEndPoint branch Mar 5, 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.