Repository navigation
Conversation
b757708 to
89400fa
Compare
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)
89400fa to
968cf5f
Compare
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM, thx!
left a minor comment
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.
c6f197c to
55d33f0
Compare
|
@samuelkarp Sorry for the delayed response, I've addressed the Copilot review comments. Requesting another round of review |
|
@tariq1890 We've had several issues in this replacement implementation which at least I missed totally in the previous review round, some potential divergences from var tests = []struct {
in string
out string
}{
{"bad", ""},
{"v1-alpha.beta.gamma", ""},
{"v1-pre", ""},
{"v1+meta", ""},
{"v1-pre+meta", ""},
{"v1.2-pre", ""},
{"v1.2+meta", ""},
{"v1.2-pre+meta", ""},
{"v1.0.0-alpha", "v1.0.0-alpha"},
{"v1.0.0-alpha.1", "v1.0.0-alpha.1"},
{"v1.0.0-alpha.beta", "v1.0.0-alpha.beta"},
{"v1.0.0-beta", "v1.0.0-beta"},
{"v1.0.0-beta.2", "v1.0.0-beta.2"},
{"v1.0.0-beta.11", "v1.0.0-beta.11"},
{"v1.0.0-rc.1", "v1.0.0-rc.1"},
{"v1", "v1.0.0"},
{"v1.0", "v1.0.0"},
{"v1.0.0", "v1.0.0"},
{"v1.2", "v1.2.0"},
{"v1.2.0", "v1.2.0"},
{"v1.2.3-456", "v1.2.3-456"},
{"v1.2.3-456.789", "v1.2.3-456.789"},
{"v1.2.3-456-789", "v1.2.3-456-789"},
{"v1.2.3-456a", "v1.2.3-456a"},
{"v1.2.3-pre", "v1.2.3-pre"},
{"v1.2.3-pre+meta", "v1.2.3-pre"},
{"v1.2.3-pre.1", "v1.2.3-pre.1"},
{"v1.2.3-zzz", "v1.2.3-zzz"},
{"v1.2.3", "v1.2.3"},
{"v1.2.3+meta", "v1.2.3"},
{"v1.2.3+meta-pre", "v1.2.3"},
{"v1.2.3+meta-pre.sha.256a", "v1.2.3"},
} |
I'd suggest pulling in the test corpus from mod/x/semver and making sure our comparison implementation agrees semver.Compare() for the relevant cases.
55d33f0 to
d93ab60
Compare
d93ab60 to
0495427
Compare
d8e81fd to
8b6c661
Compare
Signed-off-by: Tariq Ibrahim <tibrahim@nvidia.com>
8b6c661 to
18c367c
Compare
|
@klihub I've added the |


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.