Skip to content

remove containerd as dependency#51

Merged
mikebrow merged 1 commit intocontainerd:mainfrom
thaJeztah:decouple
Sep 15, 2023
Merged

remove containerd as dependency#51
mikebrow merged 1 commit intocontainerd:mainfrom
thaJeztah:decouple

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

No description provided.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Sep 13, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.11% ⚠️

Comparison is base (976af01) 64.61% compared to head (3da0047) 64.50%.

❗ Current head 3da0047 differs from pull request most recent head da8a7e5. Consider uploading reports for the commit da8a7e5 to get more accurate results

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              

see 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@thaJeztah thaJeztah force-pushed the decouple branch 2 times, most recently from 9019f6d to 514eb96 Compare September 13, 2023 08:49
@thaJeztah

This comment was marked as outdated.

@thaJeztah

This comment was marked as resolved.

@thaJeztah

This comment was marked as resolved.

@thaJeztah
Copy link
Copy Markdown
Member Author

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;

@thaJeztah
Copy link
Copy Markdown
Member Author

And opened a separate PR for the first commit;

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah marked this pull request as ready for review September 13, 2023 21:38
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
Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Member

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.

@thaJeztah
Copy link
Copy Markdown
Member Author

@klihub @samuelkarp I moved this one out of draft 👍

Copy link
Copy Markdown
Member

@klihub klihub left a 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.

@thaJeztah
Copy link
Copy Markdown
Member Author

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)

@klihub
Copy link
Copy Markdown
Member

klihub commented Sep 14, 2023

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.

@thaJeztah
Copy link
Copy Markdown
Member Author

no worries! I think I understood it was not a blocker, so no harm done!

Copy link
Copy Markdown
Member

@dmcgowan dmcgowan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Copy link
Copy Markdown
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mikebrow mikebrow merged commit f69b90b into containerd:main Sep 15, 2023
@thaJeztah thaJeztah deleted the decouple branch September 15, 2023 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants