-
Notifications
You must be signed in to change notification settings - Fork 346
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
skip cgroup v2 test of oci-tools #1406
Conversation
Signed-off-by: utam0k <k0ma@utam0k.jp>
9e47a52
to
ce40384
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1406 +/- ##
==========================================
- Coverage 68.82% 68.81% -0.01%
==========================================
Files 119 119
Lines 12779 12779
==========================================
- Hits 8795 8794 -1
- Misses 3984 3985 +1 |
if [ 0 -eq $(grep "# cgroupv2 is not supported yet " $logfile | wc -l ) ]; then | ||
echo "Skip $case bacause oci-runtime-tools doesn't support cgroup v2" | ||
continue; | ||
fi | ||
cat $logfile | ||
exit 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.
Hey, I'm not sure I understand the purpose of this part. If we are checking for the test in check_environment
function above, why do we need this here? Also, shouldn't the 0 -eq ...
part be 0 -ne ...
instead? If the grep finds occurrence, then wc will output a non-zero number, in which case we should be entering the condition, right?
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.
The logic is that if the test fails and the cause of the failure is not supported by the test in cgroup v2, the failure is ignored. I could have used check_environment, but I added this because of the uniform error messages.
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 👍
Hey, @utam0k sorry for the delay, this looks good, so I'll go ahead and merge |
Ignore the failure because it cannot be tested in a cgroup v2 environment. For example, it fails with ubuntu 22.04.