-
Notifications
You must be signed in to change notification settings - Fork 696
Refactor the version package #2735
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
Conversation
6b9869d
to
c32dbd9
Compare
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
c32dbd9
to
e1e276f
Compare
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
4414bec
to
9b60a92
Compare
Signed-off-by: Keith Zantow <kzantow@gmail.com>
Signed-off-by: Keith Zantow <kzantow@gmail.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
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.
Pull Request Overview
This PR refactors the grype/version
package to unify comparison logic, centralize constraint parsing, and simplify version comparator implementations without changing external behavior.
- Consolidated all
Compare
logic inVersion.Compare
, removed internalrich
struct - Introduced
newGenericConstraint
inGetConstraint
to centralize constraint creation - Updated individual format comparators (
deb
,apk
,bitnami
) to value receivers and uniform error handling
Reviewed Changes
Copilot reviewed 20 out of 71 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
grype/version/fuzzy_constraint.go | Expanded semver regex, switched to value receivers, updated fuzzy constraint parsing logic |
grype/version/error.go | Added ErrUnsupportedVersion , UnsupportedComparisonError , NonFatalConstraintError , invalidFormatError |
grype/version/deb_version.go | Refactored debVersion to value receiver, removed rich usage, unified error for nil versions |
grype/version/bitnami_version.go | Refactored bitnamiVersion to value receiver with semantic fallback on parse failure |
grype/version/apk_version.go | Refactored apkVersion to value receiver, unified ErrNoVersionProvided handling |
grype/version/constraint.go | Centralized all format-specific constraint creation behind newGenericConstraint in GetConstraint |
grype/search/version_constraint.go | Updated to catch UnsupportedComparisonError instead of old UnsupportedFormatError |
grype/presenter/models/vulnerability.go | Changed sortVersions to ascending order and updated in-code comments |
Comments suppressed due to low confidence (2)
grype/version/apk_version_test.go:11
- [nitpick] The test name is inconsistent with other formats; rename to
TestApkVersion_Constraint
to matchTestDebVersion_Constraint
andTestBitnamiVersion_Constraint
.
func TestVersionApk_Constraint(t *testing.T) {
grype/version/error.go:13
- The errors package is used for errors.New and errors.As but isn’t imported; add
import "errors"
to this file.
var ErrNoVersionProvided = errors.New("no version provided for comparison")
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
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.
LGTM 👍
While I was working on adding RHEL EUS support, I ran across some inconsistencies when attempting to use the
grype/version.semanticVersion
, and when digging deeper, it was apparent that most version structs in the package had similar issues:v1 vs v2
and others didv2 vs v1
. Eventually, based on the values ofversionObj.Format
, the underlying object structs, and the implementation within eachCompare()
function, the results were correct, but hard to reason which ordering was correct.finalizeComparisonVersion
within theirCompare
method, which can easily be overlooked. Ideally this should be a single choke point in this package where this is done since all of the underlying types are not exported.versionObj.Format
field deep within each type during comparison even though its the underlying state of therich
data structure that really drives this. Since there was no single source of truth on this, different implementations would choose which truth to lean on when making decisions, which made checking which behaviors were right more difficult.This lead to refactoring much of the
grype/version
package; here are the summary of changes:rich
struct in favor of type asserting specificversion.comparator
implementations.v1 vs v2
comparison (instead of needing to flip this).finalizeComparisonVersion
logic to be done only inversion.Version.Compare()
(not within each version object implementation).genericConstraint
object (less code).versions.NewVersion(value, format)
so that we are getting a sense of the objects behavior as seen by callers of this package.