fix: do not exit code 1 with invalid glibc #6342
Conversation
j-luong
force-pushed
the
fix/cli-1230_glibcChecker
branch
2 times, most recently
from
4027467 to
7546c5a
Compare
| // ValidateGlibcVersion checks if the glibc version is supported and returns an Error Catalog error if it is not. | ||
| // This check only applies to glibc-based Linux systems (amd64, arm64). | ||
| // Optionally accepts a custom GlibcVersion, mainly for testing. | ||
| func ValidateGlibcVersion(opt ...GlibcVersion) error { |
There was a problem hiding this comment.
Suggestion: Move the glibc validation logic and constants close to the legacycli workflow, since the requirement comes from there. The logic to return the glibc version can be here.
| } | ||
|
|
||
| // detectGlibcVersion attempts to detect the glibc version on Linux systems | ||
| func detectGlibcVersion() (string, error) { |
There was a problem hiding this comment.
Suggestion: I think we do have plenty of examples on the structure of the ldd output, maybe we can improve the implementation by using this knowledge and adding tests for the different cases? Adding tests for the extraction logic would require it to be callable with a string though. But this would actually implement IOSP. So probably worth it.
j-luong
force-pushed
the
fix/cli-1230_glibcChecker
branch
5 times, most recently
from
f10b176 to
0b9dd34
Compare
j-luong
force-pushed
the
fix/cli-1230_glibcChecker
branch
from
0b9dd34 to
20b3987
Compare
| } | ||
|
|
||
| version, err := versionFn() | ||
| if err != nil { |
There was a problem hiding this comment.
Question: Do we really want to fail the validation, if there was an error in the version function? I would think this could be to strict and fail more often then we want to. IMO it would be better to not fail the validation if any intermediate step fails.
| } | ||
|
|
||
| res, err := utils.SemverCompare(version, minVersion) | ||
| if err != nil { |
There was a problem hiding this comment.
same question about failing as above
| // parseSemver("2.27") // returns []int{2, 27} | ||
| // parseSemver("2.31") // returns []int{2, 31} | ||
| // parseSemver("2.35") // returns []int{2, 35} | ||
| func parseSemver(v string) ([]int, error) { |
There was a problem hiding this comment.
Suggestion: since semver is actually a bit more advanced than just 1.2.3 should we maybe use a library to do proper parsing and comparison?
There was a problem hiding this comment.
depending on the decision, if we keep our own implementation, let's add some tests for parseSemver() and SemverCompare().
| "sync" | ||
| ) | ||
|
|
||
| const ( |
There was a problem hiding this comment.
Suggestion: Move this to legacycli, as it is defined by the nodejs runtime which is part of the legacycli workflow.
| // SemverCompare("2.27", "2.31") // returns -1 | ||
| // SemverCompare("2.31", "2.31") // returns 0 | ||
| // SemverCompare("2.35", "2.31") // returns 1 | ||
| func SemverCompare(v1 string, v2 string) (int, error) { |
There was a problem hiding this comment.
Question: It took me a while to fully understand the behaviour and the usage, so I'm wondering if a simpler function like SemverGreaterEquals() bool or SemverLessThan() bool is easier to understand.
Pull Request Submission Checklist
are release-note ready, emphasizing
what was changed, not how.
What does this PR do?
This PR fixes an issue where an unsupported
glibcversion will result in a generic error catalog error and an exit code 1 for some Snyk commands.We will not exit code 2 and provide a more useful
RequirementsNotMeterror catalog error.Where should the reviewer start?
How should this be manually tested?
What's the product update that needs to be communicated to CLI users?