-
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
remove containerd as dependency #51
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## main #51 +/- ##
==========================================
- Coverage 64.61% 64.50% -0.11%
==========================================
Files 9 9
Lines 1834 1834
==========================================
- Hits 1185 1183 -2
- Misses 498 500 +2
Partials 151 151 ☔ View full report in Codecov by Sentry. |
9019f6d
to
514eb96
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
2d9fa97
to
9340ca0
Compare
Alright; all green now! I can move the first commit to a separate PR for visibility; also did a quick try in containerd if everything looks good, and so far (CI still running) I think it does; |
And opened a separate PR for the first commit; |
Signed-off-by: Sebastiaan van Stijn <[email protected]>
github.com/containerd/ttrpc v1.1.1-0.20220420014843-944ef4a40df3 | ||
github.com/moby/sys/mountinfo v0.6.2 | ||
github.com/onsi/ginkgo/v2 v2.5.0 | ||
github.com/onsi/gomega v1.24.0 | ||
github.com/opencontainers/runtime-spec v1.0.3-0.20220825212826-86290f6a00fb | ||
github.com/opencontainers/runtime-tools v0.9.0 |
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.
Does anyone know if we still need the replace rule for this one?
replace github.com/opencontainers/runtime-tools v0.9.0 => github.com/opencontainers/runtime-tools v0.0.0-20221026201742-946c877fa809
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.
I think we do. v0.9.0 is still the tip of tags.
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.
But I suspect we could also replace the replace with a more straightforward import of github.com/opencontainers/runtime-tools v0.9.1-0.20221107090550-2e043c6bd626
.
@klihub @samuelkarp I moved this one out of draft 👍 |
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.
I think it should be fine to change the old v0.1.0 Client interface. The only user used to be containerd 1.6 and now the only one is supposed to be the NRI v0.1.0 adapter plugin.
Happy to do follow-ups where needed! I honestly am not familiar with this module (at all!) and my changes were really just to simplify containerd's complex dependency tree (and to cut circular references where possible) |
Sorry if my comment was confusing. I think these changes are fine. |
no worries! I think I understood it was not a blocker, so no harm done! |
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.
Thanks!
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
No description provided.