Conversation
b757708 to
89400fa
Compare
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
Thanks! I like this; I think this looks like a reasonable alternative for this repo.
I found some issues, and left some suggestions; feel free to take those and amend your commit (no need to use a second commit for it, because we'd probably ask you to squash those anyway)
| var major, minor, patch string | ||
| var rest string | ||
| if strings.Contains(s, ".") { | ||
| major, rest, _ = strings.Cut(s, ".") | ||
| } else { | ||
| major = s | ||
| } | ||
|
|
||
| if strings.Contains(rest, ".") { | ||
| minor, patch, _ = strings.Cut(rest, ".") | ||
| } else { | ||
| minor = rest | ||
| } |
There was a problem hiding this comment.
Minor nit; I think the strings.Contains should be redundant here; strings.Cut would just be a no-op, so we can simplify;
major, rest, _ := strings.Cut(s, ".")
minor, patch, _ := strings.Cut(rest, ".")That also makes pre-declaring the var major, minor, patch redundant.
(bad values will shuffle out later when trying to parse as an integer)
89400fa to
968cf5f
Compare
| minor int | ||
| patch int | ||
| pre string | ||
| build string |
There was a problem hiding this comment.
not a blocker, but I think at a quick glance, we don't use the build info (at least we don't compare it, and I think we discard it when printing as string)
if that's the case, we can drop this field, and just discard the information
There was a problem hiding this comment.
That's totally fair.
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM, thx!
left a minor comment
Signed-off-by: Tariq Ibrahim <tibrahim@nvidia.com>
968cf5f to
f36f6f7
Compare
|
@thaJeztah Thank you for the lightning fast reviews! |
|
Thanks! |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
SemVer parsing and prerelease ordering have correctness issues, and one module contains unrelated dependency upgrades.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (2)
What changed in this PR
Replaces golang.org/x/mod/semver with a local SemVer implementation.
Changes:
- Adds local version parsing, comparison, and tests.
- Removes
x/modmetadata across modules. - Refreshes
network-device-injectordependencies.
| File | Summary |
|---|---|
plugins/writable-cgroups/go.sum |
Removes dependency checksums. |
plugins/writable-cgroups/go.mod |
Removes x/mod. |
plugins/wasm/go.sum |
Removes dependency checksums. |
plugins/wasm/go.mod |
Removes x/mod. |
plugins/ulimit-adjuster/go.sum |
Removes dependency checksums. |
plugins/ulimit-adjuster/go.mod |
Removes x/mod. |
plugins/template/go.sum |
Removes dependency checksums. |
plugins/template/go.mod |
Removes x/mod. |
plugins/rdt/go.sum |
Removes dependency checksums. |
plugins/rdt/go.mod |
Removes x/mod. |
plugins/network-logger/go.sum |
Removes dependency checksums. |
plugins/network-logger/go.mod |
Removes x/mod. |
plugins/network-device-injector/go.sum |
Refreshes dependency checksums. |
plugins/network-device-injector/go.mod |
Removes x/mod and updates dependencies. |
plugins/logger/go.sum |
Removes dependency checksums. |
plugins/logger/go.mod |
Removes x/mod. |
plugins/hook-injector/go.sum |
Removes dependency checksums. |
plugins/hook-injector/go.mod |
Removes x/mod. |
plugins/differ/go.sum |
Removes dependency checksums. |
plugins/differ/go.mod |
Removes x/mod. |
plugins/device-injector/go.sum |
Removes dependency checksums. |
plugins/device-injector/go.mod |
Removes x/mod. |
pkg/version/version.go |
Adds local SemVer parsing and comparison. |
pkg/version/version_test.go |
Tests version comparison. |
go.sum |
Removes x/mod checksums. |
go.mod |
Removes the x/mod dependency. |
examples/go.sum |
Removes x/mod checksums. |
examples/go.mod |
Removes the x/mod dependency. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| return 1 | ||
| } | ||
| } | ||
| return cmp.Compare(aVer.pre, bVer.pre) |
| func parseVersion(s string) (version, error) { | ||
| major, rest, _ := strings.Cut(s, ".") | ||
| minor, patch, _ := strings.Cut(rest, ".") | ||
|
|
| return 1 | ||
| } | ||
| } | ||
| return cmp.Compare(aVer.pre, bVer.pre) |
| func parseVersion(s string) (version, error) { | ||
| major, rest, _ := strings.Cut(s, ".") | ||
| minor, patch, _ := strings.Cut(rest, ".") | ||
|
|
Motivation
golang.org/x/modis a pretty big dependency as its scope extends beyond semver. Considering that the semver parsing requirements are minimal, importing a third party module may not be all that necessary.With this change, we decrease the CVE surface further as
golang.org/x/modis known to have CVEs. Moreover, this change shaves off one dependency from the dep-tree which is a win.