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

sso-{auth,proxy}: adding version flag #139

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

zunayed
Copy link

@zunayed zunayed commented Jan 10, 2019

pr for #36

Solution

Using LDFLAGS for the go version and looking up the version variable or the binary version

@mreiferson mreiferson self-requested a review January 11, 2019 19:38
Copy link
Contributor

@mreiferson mreiferson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great start, thanks for opening the PR @zunayed! ❤️

@@ -1,18 +1,20 @@
version := "v1.1.0"

commit := $(shell git rev-parse --short HEAD)
version := $(shell go version)
ldflags := '-X "main.goVersion=$(version)"'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go has a runtime.Version() func in the standard library that we can use instead of passing a variable externally into the build process.

func init() {
log.SetServiceName("sso-authenticator")
}

func main() {
if len(os.Args) > 1 && os.Args[1] == "version" {
fmt.Println(auth.VERSION)
fmt.Println(goVersion)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aesthetic and nit-picky, but my preference for this would be:

sso-{proxy,auth} vX.XX (built w/goX.XX)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd just like to reiterate my request in #36 (comment) that the app version and go version be split across two lines for easier tooling.

In my (fairly limited) experience, the thing that matters when building automation is the app version, and it's a bit annoying to have to do extra work to split it out from the go version.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, sorry I missed that in the original issue.

I get it, and can probably get over my distaste. I feel like you would still have to parse it, e.g. by grabbing just the first line of output, but yea, that is easier.

func init() {
log.SetServiceName("sso-authenticator")
}

func main() {
if len(os.Args) > 1 && os.Args[1] == "version" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see this as an actual command line flag (i.e. sso-auth --version), rather than a manually-parsed argument. Primarily, my thinking is that it if we implement it that way, it'll automatically show up in sso-auth --help output, so users will know it's an option.

Copy link
Contributor

@mreiferson mreiferson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great start, thanks for opening the PR @zunayed! ❤️

@mreiferson mreiferson added the enhancement New feature or request label Jan 11, 2019
@mreiferson mreiferson changed the title Adding version flag to both sso-auth and sso-proxy *: adding version flag Jan 11, 2019
@mreiferson mreiferson changed the title *: adding version flag sso-{auth,proxy}: adding version flag Jan 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants