-
Notifications
You must be signed in to change notification settings - Fork 716
logging: ensure that MagicArgv1 is always argv1 #1056
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
Conversation
Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
Fix issue 1054 Replace PR 1055 Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
Zheaoli
left a comment
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
| return nil, err | ||
| } | ||
| args := map[string]string{ | ||
| logging.MagicArgv1: dataStore, |
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.
An immature idea, why not consider splicing the parameters behind the dataStore, like
binary:///usr/local/bin/nerdctl?_NERDCTL_INTERNAL_LOGGING=/var/lib/nerdctl/1935db59\?maxSize=1
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.
Just didn't want to invent a new microformat
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.
Or put args in the labels of the container? like nerdctl/logging.driver / nerdctl/logging.max-file
To be honest, I am not inclined to store some information by adding new files. :(
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.
How can we let the log binary access the labels?
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.
It's a bit tricky, ignore it
junnplus
left a comment
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
|
@AkihiroSuda would you plz merge :) |
Fix #1054
Replace #1055
The opts are now stored in
filepath.Join(dataStore, "containers", ns, id, "log-config.json")