Update containerd log dependency #1898
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
What needs to be done to get the workflows started here? |
ambarve
reviewed
Sep 25, 2023
| @@ -14,7 +14,7 @@ import ( | |||
|
|
|||
| "github.com/Microsoft/go-winio" | |||
| task "github.com/containerd/containerd/api/runtime/task/v2" | |||
| "github.com/containerd/containerd/log" | |||
There was a problem hiding this comment.
Is there a reason why we use containerd/log here instead of internal/log? (I think this is more of a question for @kevpar)
There was a problem hiding this comment.
That's fair. @dmcgowan would you mind removing this import altogether, and changing the log.RFC3339NanoFixed in this file to hcslog.TimeFormat?
There was a problem hiding this comment.
Updated. The test package is still using github.com/containerd/log, it seems isolated but maybe it could just use logrus directly in the future.
There was a problem hiding this comment.
Agreed, we can try to move that in the future
Signed-off-by: Derek McGowan <derek@mcg.dev>
dmcgowan
force-pushed
the
update-containerd-log-dependency
branch
from
eaedc56
to
8b69b76
Compare
|
Can I get the workflows restarted here? |
ambarve
approved these changes
Sep 29, 2023
kevpar
approved these changes
Sep 29, 2023
|
CI re-run looks good. |
akhilerm
added a commit
to akhilerm/containerd
that referenced
this pull request
Jan 26, 2024
The log package was kept because hcsshim had a dependency. This was removed in microsoft/hcsshim#1898. So, its not required to maintain the containerd/containerd/log package anymore. Signed-off-by: Akhil Mohan <akhilerm@gmail.com>
juliusl
pushed a commit
to juliusl/containerd
that referenced
this pull request
Jan 26, 2024
The log package was kept because hcsshim had a dependency. This was removed in microsoft/hcsshim#1898. So, its not required to maintain the containerd/containerd/log package anymore. Signed-off-by: Akhil Mohan <akhilerm@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Moving the log package to a separate repository is part of our containerd 2.0 effort to break circular dependencies where we can. Importing the
logpackage in the internal log seems unnecessary since it only uses a const. If copying that const is still not ideal, it can be updated to use new package as well (removed based on "A little copying is better than a little dependency.")Uses the new log package for the shim and tests.