Skip to content

Add version package #37

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

Merged
merged 2 commits into from
Dec 14, 2020
Merged

Add version package #37

merged 2 commits into from
Dec 14, 2020

Conversation

lev-ok
Copy link
Contributor

@lev-ok lev-ok commented Dec 14, 2020

I would want to add only one package version. To be honest I don't like keep VERSION in main.go.
Reasons:

  1. VERSION - uppercase
  2. This forces to always add an excess arg cli.NewApp(VERSION)

It's easy to make DevOps compatible with both implementations. Just try to rewrite in both places:
-ldflags "-X main.VERSION=v1.0.0 -X github.com/gruntwork-io/gruntwork-cli/version.VERSION=v1.0.0"

Copy link
Member

@brikis98 brikis98 left a comment

Choose a reason for hiding this comment

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

Ah, so the only difference is that, when setting the version number in a CLI command, you do so on version.VERSION instead of main.VERSION? If so, that's fine by me, and does reduce boilerplate a bit for each app.

Could you update the README accordingly?

@lev-ok
Copy link
Contributor Author

lev-ok commented Dec 14, 2020

Exactly. When it's in a separate package, we have the ability to call version.Version() from different parts of the code, without worrying about where the magic actually happens.

Done!

Copy link
Member

@brikis98 brikis98 left a comment

Choose a reason for hiding this comment

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

LGTM! I'll kick off tests shortly.

@brikis98
Copy link
Member

Tests passed! Merging now.

@brikis98 brikis98 merged commit 6fc74d4 into gruntwork-io:master Dec 14, 2020
@brikis98
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants