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

fix: version command fix #149

Merged
merged 8 commits into from
Aug 23, 2022
Merged

fix: version command fix #149

merged 8 commits into from
Aug 23, 2022

Conversation

james-milligan
Copy link
Contributor

Possible fix for populating version variables when using go install
Signed-off-by: James-Milligan james@omnant.co.uk

Signed-off-by: James-Milligan <james@omnant.co.uk>
@james-milligan james-milligan marked this pull request as ready for review August 19, 2022 11:13
@AlexsJones
Copy link
Member

Does the "vcs.revision" print the short sha?

Signed-off-by: James-Milligan <james@omnant.co.uk>
@james-milligan
Copy link
Contributor Author

Does the "vcs.revision" print the short sha?

it does not, example output:

flagd 0.0.9 (40676e362da172315346db7ef7e5b97a0cabb319) built at 2022-08-19T13:14:24Z

Copy link
Member

@AlexsJones AlexsJones left a comment

Choose a reason for hiding this comment

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

This seems to be slightly confusing, if its not "devel" version it will use the vcs.time value from the build info? What about the params in main?

@james-milligan
Copy link
Contributor Author

The version key value from ReadBuildInfo() will currently read v0.0.9 if go install was used to install the binary, but will read (devel) if go get was used. This means that the priority is currently: go 1.18 build info => -ldflags => default values in main.

As for the remaining values (time and revision) I felt it fitting to pull these from the same data set as the version, so when version is set and not (devel) it'll use the vcs values.

Copy link
Member

@AlexsJones AlexsJones left a comment

Choose a reason for hiding this comment

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

This isn't working as expected when producing a docker image

❯ docker run cnskunkworks/flagd:james-branch version

		 ______   __       ________   _______    ______
		/_____/\ /_/\     /_______/\ /______/\  /_____/\
		\::::_\/_\:\ \    \::: _  \ \\::::__\/__\:::_ \ \
		 \:\/___/\\:\ \    \::(_)  \ \\:\ /____/\\:\ \ \ \
		  \:::._\/ \:\ \____\:: __  \ \\:\\_  _\/ \:\ \ \ \
		   \:\ \    \:\/___/\\:.\ \  \ \\:\_\ \ \  \:\/.:| |
		    \_\/     \_____\/ \__\/\__\/ \_____\/   \____/_/

flagd  (5365272) built at 2022-08-22T15:59:00Z

The injected arg is --build-arg=VERSION="$$(git describe --tags --abbrev=0)"

which provides the tag locally on your repo of v0.0.1 which I created manually.

@james-milligan
Copy link
Contributor Author

just looked into this and it seems the version value remains unset in this example, which is making me lose confidence in this as an approach, a simple fix would be to replace:
if ok && details.Main.Version != "(devel)" {
with
if ok && details.Main.Version != "" && details.Main.Version != "(devel)" {
but this is making it feel a little bloated which suggests it may not be the right way to be going about this.
what are your thoughts @AlexsJones

@AlexsJones
Copy link
Member

Perhaps unless it's a downloaded release we compromise to say that go install will be

flagd dev (HEAD) built at unknown

if someone wants a supported version they download a release?

@james-milligan
Copy link
Contributor Author

Personally I would feel more comfortable with a 'hacky' approach than to not provide a version at all on the go install binary.
Ive updated the priority order so that ReadBuildInfo() is more of a final check.

What are your thoughts @skyerus @beeme1mr @toddbaert

@AlexsJones
Copy link
Member

I don't have a problem with the revised code now, this wasn't the case when we discussed it previously, so I am happy to accept it now...

@AlexsJones AlexsJones merged commit 8a85add into open-feature:main Aug 23, 2022
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.

3 participants