Skip to content
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

add 'none' logger #3633

Merged
merged 1 commit into from
Nov 5, 2024
Merged

Conversation

coderbirju
Copy link
Contributor

@coderbirju coderbirju commented Nov 1, 2024

Attempt at adding option "none" for nerdctl run with --log-driver flag.
#1039

Feedback welcome.

Testing Done:

sudo ./_output/nerdctl run \
  --log-driver=none \
  --name my-container \
  nginx  

Verified that <conatinerd-id>-json.log file is not created in
/var/lib/nerdctl/1935db59/containers/default/<container-id> folder

Verified that sudo nerdctl logs my-container exits with
log type `none` was selected, nothing to log

Ignoring hostname lifecycle.json log-config.json oci-hook.createRuntime.log resolv.conf files within
/var/lib/nerdctl/1935db59/containers/default/<container-id> folder
I believe these are not container logs and as such are not in scope of being removed with the --log-driver flag

@coderbirju coderbirju force-pushed the add-none-logger branch 2 times, most recently from b62d4b6 to b872a0d Compare November 1, 2024 20:44
@coderbirju coderbirju marked this pull request as ready for review November 1, 2024 21:07
cmd/nerdctl/main.go Outdated Show resolved Hide resolved
pkg/logging/log_viewer.go Outdated Show resolved Hide resolved
pkg/logging/none_logger_test.go Outdated Show resolved Hide resolved
Signed-off-by: Arjun Raja Yogidas <arjunry@amazon.com>
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

@AkihiroSuda AkihiroSuda merged commit 9bd0ab9 into containerd:main Nov 5, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants