Add version command to the tool to output the current version of the tool #56
Conversation
pkg/version/version.go
Outdated
| package version | ||
|
|
||
| var ( | ||
| BuildInfo = "v0.7" |
There was a problem hiding this comment.
If this version number is going to live in a new file, we will also need to update scripts/prepare-for-release. That script syncs the version numbers across all relevant files (currently only the README) whenever we cut new releases.
There was a problem hiding this comment.
BuildInfo just a placeholder value, it will get overwritten during build time by ldflags. I probably need more help in understanding the release cut process
EMBED_FLAGS=-ldflags '-X "${SIMPLE_EC2_CLOUDFORMATION_TEMPLATE_VAR}=${SIMPLE_EC2_CLOUDFORMATION_TEMPLATE_ENCODED}"\
-X "${E2E_CFN_TEST_CLOUDFORMATION_TEMPLATE_VAR}=${E2E_CFN_TEST_CLOUDFORMATION_TEMPLATE_ENCODED}"\
-X "${E2E_CONNECT_TEST_CLOUDFORMATION_TEMPLATE_VAR}=${E2E_CONNECT_TEST_CLOUDFORMATION_TEMPLATE_ENCODED}"\
-X "${E2E_EC2HELPER_TEST_CLOUDFORMATION_TEMPLATE_VAR}=${E2E_EC2HELPER_TEST_CLOUDFORMATION_TEMPLATE_ENCODED}"\
-X "${VERSION_VAR}=${VERSION}"'
There was a problem hiding this comment.
Ah, that makes sense. I'm okay with this design, but I'd suggest we use a more generic placeholder in this file so that future developers or build systems don't confuse it for an actual version number. Let's also add a comment indicating that the Makefile will fill it in, similar to how the CloudFormation variables are handled.
The way our release process works would be irrelevant at that point, but the main idea is that we have a script that creates a git tag for the new version number, updates it in any relevant human- or machine-readable portions of the repo, and then opens a PR for it on GitHub.
There was a problem hiding this comment.
Are you okey with called the place holder variable named "BUILD_VERSION_VAR" instead of "VERSION_VAR"?
There was a problem hiding this comment.
Thanks for your feedback @snay2 . Pushed with the above mentioned changes. Please review
There was a problem hiding this comment.
Thanks @icarthick!
I should clarify my earlier comment. I think we should use a generic value in the BuildInfo variable. The name of the variable is fine, but perhaps the value should be something like VERSION_PLACEHOLDER. The idea is that if we have a hardcoded version number like v0.7, it will quickly become out of date, and it may be confusing later on. If instead we have a value for the placeholder that is very obviously not a valid version number, it will be easier to understand the intent, as well as to identify if and when that placeholder ever leaks somewhere it shouldn't be.
So in addition to the other changes you've already made, can you change this line to the following? After that, I am satisfied with this PR.
BuildInfo = "VERSION_PLACEHOLDER"There was a problem hiding this comment.
Clearly, I overthought your earlier comment :) Latest commit, should have the change. Let me know if you are able to merge it
|
Looks like some of the checks are failing because go get is being used instead of go install. How do you folks intend to handle this? Is changing all those |
Yep, that's the basic idea. I made those changes in this PR: #57. After that's merged, you can rebase and the builds should pass. |
|
@snay2 I rebased this branch from main. Please merge if everything checks out |
README.md
Outdated
| @@ -100,8 +100,11 @@ Flags: | |||
| --tags stringToString The tags applied to instances and volumes at launch (Example: tag1=val1,tag2=val2) (default []) | |||
| ``` | |||
|
|
|||
| **Single Command Launch** | |||
|
|
|||
| **Single Command Version** | |||
There was a problem hiding this comment.
This section is describing how to launch an EC2 instance with a single command (as opposed to the interactive mode or a single command with flags, which are described later), so the original title is correct.
The documentation for the version command belongs in its own section, not as part of the launch subcommand. Putting it at the beginning or end of the "Examples" section would make sense to me (i.e., around line 74 or line 565).
There was a problem hiding this comment.
Updated to include your feedback
README.md
Outdated
| @@ -71,6 +71,13 @@ curl -Lo simple-ec2 https://github.com/awslabs/aws-simple-ec2-cli/releases/downl | |||
|
|
|||
| ## Examples | |||
|
|
|||
| ### Connect | |||
There was a problem hiding this comment.
Sorry for all the nit-picking. Can you rename this to Version?
There was a problem hiding this comment.
It is i who has to apologize for being this careless, thanks for your patience. Made the requested change
README.md
Outdated
| @@ -80,7 +87,7 @@ $ simple-ec2 launch -h | |||
| Launch an Amazon EC2 instance with the default configurations. All configurations can be overridden by configurations provided by configuration files or user input. | |||
|
|
|||
| Usage: | |||
| simple-ec2 launch [flags] | |||
| simple-ec2 launch|version [flags] | |||
There was a problem hiding this comment.
Can you remove |version now that it has its own section?
There was a problem hiding this comment.
Pushed the change
Issue # 45, if available:
#45
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.