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

Get current version from git tag #390

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Conversation

facuMH
Copy link
Contributor

@facuMH facuMH commented Jan 14, 2025

This PR adds a mechanism that uses the git tag (if any) as release version. Hence preventing the task to update both the version file as well as the release tag in git. It also should check if there have been any commits since the last label, and if the build is a dirty build (aka has uncommitted changes)

the git tag is "v2.0.1" => Major=2, Minor=0, Patch=1, Meta=""
the git tag is "v2.0.1-rc1" => Major=2, Minor=0, Patch=1, Meta="rc1"
the git tag is "v2.0.1" but there are extra commits on top: => Major=2, Minor=0, Patch=1, Meta="dev"
the git tag is "v2.0.1-rc1" with extra commits on top => Major=2, Minor=0, Patch=1, Meta="rc1-dev"
there is no git tag (e.g. development) => handle like "v0.0.0" with extra commits on top

@facuMH facuMH requested a review from LuisPH3 January 14, 2025 13:58
@facuMH facuMH self-assigned this Jan 14, 2025
Copy link
Collaborator

@HerbertJordan HerbertJordan left a comment

Choose a reason for hiding this comment

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

Looks good. However, I added some minor code changes to reduce mutable globals. Please have a look.

version/version_test.go Outdated Show resolved Hide resolved
version/version.go Outdated Show resolved Hide resolved
version/version_test.go Outdated Show resolved Hide resolved
version/version.go Outdated Show resolved Hide resolved
version/version.go Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
vsn += "-" + gitDate
}
return vsn
}

func AsString() string {
// meta is not used for now, so we ignore it.
versionMajor, versionMinor, versionPatch, _ := parseVersion(Version)
Copy link
Collaborator

Choose a reason for hiding this comment

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

By moving the version number parsing from the init code to a lazy-evaluated place, you effectively made the final sonicd version command in the Makefile for checking whether the version string could be correctly parsed useless -- since it is just printing the Version constant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

even if parseVersion is called in init, it just initializes the 3 version variables, which are only used when comparing version with the network. Both sonicd and sonictool when running version cmd, they just return the Version. I can change it back to have parseVersion in the init, but it does not affect the version cmds

Copy link
Collaborator

Choose a reason for hiding this comment

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

If it is in the init it does affect the version command in the sense that it gets executed. And if there is a parsing error in the version number that causes the parser to panic because of a very miss-shaped version tag, then we would detect it immediately at the end of the build.

The way it is now, we would only see it crashing if AsString() is called, which basically never happens. Would be bad if this would happen the first time in production.

LuisPH3
LuisPH3 previously approved these changes Jan 16, 2025
@facuMH facuMH requested a review from HerbertJordan January 16, 2025 09:09
Copy link
Collaborator

@HerbertJordan HerbertJordan left a comment

Choose a reason for hiding this comment

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

This PR still needs a fix making sure that the version parsing of a release build is detected early -- not just while running in production.

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