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(update): strip 'v' from version string #334

Merged
merged 2 commits into from
Jun 17, 2022

Conversation

ZyanKLee
Copy link
Contributor

for comparing the version strings with semver (and only there), we need to strip the 'v' prefix from the version strings.

for comparing the version strings with semver (and only there), we need to strip the 'v' prefix from the version strings.
@AXDOOMER AXDOOMER self-requested a review June 17, 2022 01:42
Copy link
Collaborator

@AXDOOMER AXDOOMER left a comment

Choose a reason for hiding this comment

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

You'll get an index out of range in update.go because the code doesn't check for errors before making the semver. You can test this by disabling your internet connection.

Also, I'd prefer strings.TrimLeft(verstr, "v"). This is better than blindly stripping the first char.

TrimLeft is more reliable as it won't crash if the string is empty
Copy link
Collaborator

@AXDOOMER AXDOOMER left a comment

Choose a reason for hiding this comment

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

I replaced the slice operators with a left trim of the 'v' character.

@ZyanKLee
Copy link
Contributor Author

I'll have a last check if it actually really works now, then I will merge it and tag 0.12.2

@ZyanKLee
Copy link
Contributor Author

everything works now ... yeah!

@ZyanKLee ZyanKLee merged commit 0f1765c into master Jun 17, 2022
@ZyanKLee ZyanKLee deleted the bugfix/semver-comparison-version-strings branch June 17, 2022 10:23
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.

None yet

3 participants