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

feat: add version library #12

Merged
merged 1 commit into from
Jun 20, 2024
Merged

feat: add version library #12

merged 1 commit into from
Jun 20, 2024

Conversation

starbops
Copy link
Member

The Harvester Version library augments the widelyused SemVer package, adding Harvester-specific logic, especially for Harvester upgrades. It is meant to be consumed by the Harvester controller and command-line helper tool.

Related issue: harvester/harvester#2877

Copy link
Member

@brandboat brandboat left a comment

Choose a reason for hiding this comment

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

overall LGTM, just some minor comments

version/version.go Outdated Show resolved Hide resolved
version/version.go Outdated Show resolved Hide resolved
version/upgrade_test.go Show resolved Hide resolved
Comment on lines +50 to +57
// Upgrading to dev versions is always allowed
if u.upgradeVersion.isDev {
return nil
}
Copy link
Member

Choose a reason for hiding this comment

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

If "strictMode=true", we can still upgrade from one dev version to another dev version. It's a bit weird to me due to the word "strict", but if we are doing this for specific purpose, please ignore this comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is open to discussion. From a developer perspective, I tend to allow this kind of upgrade path (dev-to-dev upgrades) by default for the convenience of both developers and QA—actually, the part that leverages this library never bothers setting the strict mode off. It just checks the skip-check annotation and skip through the entire eligibility check, making the mode seem redundant. cc @bk201

Copy link
Member Author

Choose a reason for hiding this comment

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

After discussion, the strict-mode for upgrade will be introduced as a new Harvester setting. I'll create a new ticket to track the progress. Thanks!

version/upgrade.go Outdated Show resolved Hide resolved
@starbops starbops force-pushed the enh-2877 branch 2 times, most recently from 5dca299 to 7eac1e2 Compare June 18, 2024 08:03
return nil
}

// Same-version upgrade is always allowed
Copy link
Member

Choose a reason for hiding this comment

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

Do we prohibit this in the current upgrade code?

Copy link
Member Author

Choose a reason for hiding this comment

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

As I know, no. Why the ask?

The Harvester Version library augments the widelyused SemVer package,
adding Harvester-specific logic, especially for Harvester upgrades. It
is meant to be consumed by the Harvester controller and command-line
helper tool.

Signed-off-by: Zespre Chang <[email protected]>
@starbops starbops merged commit eb4251a into harvester:main Jun 20, 2024
4 checks passed
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