-
Notifications
You must be signed in to change notification settings - Fork 62
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 guardrails for parsing OS version #619
Conversation
Signed-off-by: h4l0gen <ks3913688@gmail.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #619 +/- ##
==========================================
+ Coverage 32.51% 33.92% +1.41%
==========================================
Files 17 18 +1
Lines 1621 1518 -103
==========================================
- Hits 527 515 -12
+ Misses 1062 970 -92
- Partials 32 33 +1 ☔ View full report in Codecov by Sentry. |
Hey sorry, as per this we need to run golangci-lint too, i will run it and make required changes if any. |
Signed-off-by: Kapil Sharma <ks3913688@gmail.com>
osVersion := strings.Join(versionParts[:2], ".") | ||
return "ubuntu", osVersion, nil | ||
} | ||
return "", "", fmt.Errorf("invalid Ubuntu version format: %s", osData["VERSION_ID"]) |
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.
can we add this check in getAptImageName if the os.Type is Debian instead?
@@ -301,34 +301,40 @@ func patchWithContext(ctx context.Context, ch chan error, image, reportFile, pat | |||
return eg.Wait() | |||
} | |||
|
|||
func getOSType(ctx context.Context, osreleaseBytes []byte) (string, error) { | |||
func getOSType(ctx context.Context, osreleaseBytes []byte) (string, string, error) { |
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.
since a lot of the work is repeated with getOSVersion, maybe we can combine the two functions into one and remove getOSVersion?
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.
sure, looking into it.
closing due to #736 |
adding guardrails for parsing OS version as per discussion
Closes #438