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

Introduce a log-level flag. #2036

Merged
merged 4 commits into from
Jun 17, 2023
Merged

Introduce a log-level flag. #2036

merged 4 commits into from
Jun 17, 2023

Conversation

yihuaf
Copy link
Collaborator

@yihuaf yihuaf commented Jun 12, 2023

Fix #2028

As proposed in #2028, we introduce a --log-level flag to allow us to easily set log level through runtimes. To avoid confusion, we also deprecated setting log level through env variable. The --debug flag is kept for compatibility reasons.

Note, the integration test was not changed in this PR. If this PR is accepted, we will make the changes to the rust integration test to keep things consistent.

@codecov-commenter
Copy link

codecov-commenter commented Jun 12, 2023

Codecov Report

Merging #2036 (9f0f4be) into main (1a6d1f4) will decrease coverage by 0.03%.
The diff coverage is 82.05%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2036      +/-   ##
==========================================
- Coverage   64.82%   64.80%   -0.03%     
==========================================
  Files         129      129              
  Lines       14763    14765       +2     
==========================================
- Hits         9570     9568       -2     
- Misses       5193     5197       +4     

@yihuaf yihuaf marked this pull request as ready for review June 12, 2023 05:51
@yihuaf yihuaf requested review from utam0k and a team June 12, 2023 05:51
@yihuaf yihuaf self-assigned this Jun 12, 2023
@@ -24,7 +24,7 @@ if [ ! -f ${ROOT}/bundle.tar.gz ]; then
fi
touch ${LOGFILE}

sudo YOUKI_LOG_LEVEL="error" ${ROOT}/integration_test run --runtime "$RUNTIME" --runtimetest ${ROOT}/runtimetest > $LOGFILE
sudo ${ROOT}/integration_test run --runtime "$RUNTIME" --runtimetest ${ROOT}/runtimetest > $LOGFILE
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should be also passing the --log-level flag here, set to error, right?

Copy link
Collaborator Author

@yihuaf yihuaf Jun 12, 2023

Choose a reason for hiding this comment

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

No, this script is calling the integration_test binary, which don't take the log-level flag. At least I did not make the change in this PR. The actual code that calls the runtime inside the integration_test is changed to add the log-level flag. The integration_test binary uses YOUKI_INTEGRATION_LOG_LEVEL env variable. https://github.com/containers/youki/blob/1a6d1f4bd7553e971d6d787698a9732836188444/tests/rust-integration-tests/integration_test/src/logger.rs#L6

It is possible that the original intent is YOUKI_INTEGRATION_LOG_LEVEL?

@yihuaf yihuaf marked this pull request as draft June 12, 2023 22:58
@yihuaf yihuaf force-pushed the yihuaf/log-level branch from 3ae9469 to 38259a3 Compare June 13, 2023 01:18
@yihuaf yihuaf marked this pull request as ready for review June 13, 2023 01:29
.github/workflows/e2e.yaml Outdated Show resolved Hide resolved
@@ -171,3 +171,13 @@ cd ..
./youki list
./youki delete rootless_container
```

#### Log level
Copy link
Member

Choose a reason for hiding this comment

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

👍

@yihuaf yihuaf requested review from utam0k and YJDoc2 June 13, 2023 18:07
yihuaf added 3 commits June 13, 2023 13:16
In short:

Propose the default log level for release to error
Keep the --debug flag and it sets the log level to debug
Introduce --log-level flag to set the log level
--debug flag will be ignored if --log-level flag is also set.
Keep the default log level for debug build to debug

Signed-off-by: yihuaf <[email protected]>
Signed-off-by: yihuaf <[email protected]>
@yihuaf yihuaf force-pushed the yihuaf/log-level branch from bd71259 to 6cf80ee Compare June 14, 2023 05:09
@yihuaf yihuaf force-pushed the yihuaf/log-level branch from b84ec5f to 9f0f4be Compare June 15, 2023 23:22
@yihuaf
Copy link
Collaborator Author

yihuaf commented Jun 17, 2023

@utam0k removed the workflow_dispatch. PTAL.

Copy link
Member

@utam0k utam0k left a comment

Choose a reason for hiding this comment

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

💯 Thanks a lot 🙏

@utam0k utam0k merged commit ebef6db into youki-dev:main Jun 17, 2023
@yihuaf yihuaf deleted the yihuaf/log-level branch June 17, 2023 17:10
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.

[RFC] Introducing a new --log-level flag
4 participants