-
Notifications
You must be signed in to change notification settings - Fork 684
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
Add semver check for dev and beta version #5757
Conversation
Signed-off-by: pmahindrakar-oss <[email protected]>
Signed-off-by: pmahindrakar-oss <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one minor comment, otherwise LGTM.
Signed-off-by: pmahindrakar-oss <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5757 +/- ##
=========================================
Coverage ? 36.29%
=========================================
Files ? 1305
Lines ? 109997
Branches ? 0
=========================================
Hits ? 39926
Misses ? 65915
Partials ? 4156
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -133,6 +134,9 @@ var ( | |||
MaxSizeInMBForOffloading: 1000, // 1 GB is the default size before failing fast. | |||
}, | |||
} | |||
|
|||
// This regex is used to sanitize semver versions passed to IsSupportedSDK checks for literal offloading feature. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mind including an example of what this matches against with the dev sdk versions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed this. Will add in follow up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added here #5758
Why are the changes needed?
Supporting dev and beta versions of the flytekit sdk during semver version checks for literal offloading feature.
Follow up to #5705
What changes were proposed in this pull request?
Do a regex match for the version string passed against a valid semver and use that for comparision with the supported one.
How was this patch tested?
Unit test added to test dev and beta versions
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link