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.

Sign up for GitHub

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

Jump to bottom

Add debug flag #465

Merged
merged 3 commits into from
Nov 17, 2021
Merged

Add debug flag #465

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
93bbaba
feat: add debug flag
unknowndevQwQ Nov 9, 2021
475a2bf
why?
unknowndevQwQ Nov 16, 2021
5725f52
the end?
unknowndevQwQ Nov 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 33 additions & 7 deletions crates/youki/src/logger.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
//! Default Youki Logger

use anyhow::{bail, Context, Result};
use log::LevelFilter;
use std::env;
use std::fs::OpenOptions;
use std::io::Write;
use std::path::PathBuf;
use std::process::exit;
use std::str::FromStr;
use std::string::String;

/// If in debug mode, default level is debug to get maximum logging
#[cfg(debug_assertions)]
Expand All @@ -19,12 +24,34 @@ const LOG_FORMAT_JSON: &str = "json";
/// Initialize the logger, must be called before accessing the logger
/// Multiple parts might call this at once, but the actual initialization
/// is done only once due to use of OnceCell
pub fn init(log_file: Option<PathBuf>, log_format: Option<String>) -> Result<()> {
pub fn init(
log_debug_flag: bool,
log_file: Option<PathBuf>,
log_format: Option<String>,
) -> Result<()> {
let formatter = match log_format.as_deref() {
None | Some(LOG_FORMAT_TEXT) => text_write,
Some(LOG_FORMAT_JSON) => json_write,
Some(unknown) => bail!("unknown log format: {}", unknown),
};
let log_level_from_env = match env::var("YOUKI_LOG_LEVEL") {
Ok(val) => val,
Err(_) => String::from("i don't know"),
};
let log_level = if log_debug_flag {
"debug"
} else if log_level_from_env != "i don't know" {
&log_level_from_env
} else {
DEFAULT_LOG_LEVEL
};
let log_level = match LevelFilter::from_str(log_level) {
Ok(val) => val,
Err(err) => {
bail!("error: '{}', val from env: '{}'", err, log_level);
exit(1)
},
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about this?

Suggested change
let log_level_from_env = match env::var("YOUKI_LOG_LEVEL") {
Ok(val) => val,
Err(_) => String::from("i don't know"),
};
let log_level = if log_debug_flag {
"debug"
} else if log_level_from_env != "i don't know" {
&log_level_from_env
} else {
DEFAULT_LOG_LEVEL
};
let log_level = match LevelFilter::from_str(log_level) {
Ok(val) => val,
Err(err) => {
bail!("error: '{}', val from env: '{}'", err, log_level);
exit(1)
},
};
let filter: Cow<str> = if debug_flag {
"debug".into()
} else if let Ok(level) = std::env::var("YOUKI_LOG_LEVEL") {
level.into()
} else {
DEFAULT_LOG_LEVEL.into()
};
env_logger::Builder::new()
.filter_level(LevelFilter::from_str(filter.as_ref()).context("failed to parse log level")?)
.format(formatter)
.target(target)
.init();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

YOUKI_LOG_LEVEL=o sudo -E ./youki/target/x86_64-unknown-linux-gnu/release/youki state test

log init failed: failed to parse log level

Caused by:
    attempted to convert a string that doesn't match an existing log level
{
  "ociVersion": "v1.0.2",
  "id": "test",
  "status": "stopped",
  "pid": 114514,
  "bundle": "test",
  "annotations": {},
  "created": "2021-11-11T11:11:11.111111111Z",
  "creator": 0,
  "useSystemd": false
}

Is it normal to continue doing other things after a loglevel failure?

let target = if let Some(log_file) = log_file {
let file = OpenOptions::new()
.create(true)
Expand All @@ -36,12 +63,11 @@ pub fn init(log_file: Option<PathBuf>, log_format: Option<String>) -> Result<()>
} else {
env_logger::Target::Stderr
};
env_logger::Builder::from_env(
env_logger::Env::default().filter_or("YOUKI_LOG_LEVEL", DEFAULT_LOG_LEVEL),
)
.format(formatter)
.target(target)
.init();
env_logger::Builder::new()
.filter_level(log_level)
.format(formatter)
.target(target)
.init();

Ok(())
}
Expand Down
7 changes: 6 additions & 1 deletion crates/youki/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@ use nix::unistd::getuid;
#[derive(Parser, Debug)]
#[clap(version = crate_version!(), author = "youki team")]
struct Opts {
// I don't know how to get the log level when the --debug flag is not set (I want to show some default values on the help page when the options are not set)
// Example: '--debug change log level to debug. (default: "warn")'
/// change log level to debug.
Copy link
Member

Choose a reason for hiding this comment

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

These comments in this section are ./youki -h will be output.
When I thought about it, I felt that the first line was unnecessary.

Copy link
Member

Choose a reason for hiding this comment

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

These codes may be helpful for you.
https://github.com/containers/youki/blob/ba08bb323165f4d0b98a0321fb990725bb32de69/crates/youki/src/commands/create.rs#L10-L27

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These codes may be helpful for you.

https://github.com/containers/youki/blob/ba08bb323165f4d0b98a0321fb990725bb32de69/crates/youki/src/commands/create.rs#L10-L27

The default value in some options is dynamic, for example, --root

Copy link
Member

Choose a reason for hiding this comment

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

@unknowndevQwQ
My point is that the comments in this section are not for developers, but for users. I would like you to take that into account when making your comments.

Copy link
Contributor Author

@unknowndevQwQ unknowndevQwQ Nov 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@unknowndevQwQ My point is that the comments in this section are not for developers, but for users. I would like you to take that into account when making your comments.

Should I delete L41-L42?

or like this?
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@utam0k

#[clap(long)]
debug: bool,
yihuaf marked this conversation as resolved.
Show resolved Hide resolved
#[clap(short, long)]
log: Option<PathBuf>,
#[clap(long)]
Expand Down Expand Up @@ -104,7 +109,7 @@ fn main() -> Result<()> {

let opts = Opts::parse();

if let Err(e) = crate::logger::init(opts.log, opts.log_format) {
if let Err(e) = crate::logger::init(opts.debug, opts.log, opts.log_format) {
eprintln!("log init failed: {:?}", e);
}

Expand Down