-
Notifications
You must be signed in to change notification settings - Fork 944
ci: output log when install criu #2323
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
Codecov Report
@@ Coverage Diff @@
## master #2323 +/- ##
==========================================
+ Coverage 67.34% 67.37% +0.02%
==========================================
Files 218 218
Lines 17674 17674
==========================================
+ Hits 11902 11907 +5
Misses 4365 4365
+ Partials 1407 1402 -5
|
|
interesting. the change is LGTM but I didn't see any output in the CI log. 🤔 |
|
my bad. Please remove the redirection. |
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.
please remove the redirection or think other way to show the comment
ci test alway hang on installing criu things some times ago, add log to make sure who blocked the ci, see #2298. Signed-off-by: Ace-Tang <[email protected]>
|
Keep redirection, echo log before function, or log will too many, I did not consider this before. |
| os_dist="$(detect_os)" | ||
| if [[ "${os_dist}" = "Ubuntu" ]]; then | ||
| criu::ubuntu::install_dependencies > /dev/null | ||
| echo ">>>> start to download criu from github repository <<<<" |
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.
If the downloading task fails, would this script show the detailed reason why it fails? @Ace-Tang
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.
I think yes.
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.
only know we stack at downing from github.
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.
ok, then it meets my demand.
| echo ">>>> install criu <<<<" | ||
|
|
||
| os_dist="$(detect_os)" | ||
| if [[ "${os_dist}" = "Ubuntu" ]]; then |
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.
could we add one comment here?
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.
I do not think so, >>>> install criu <<<< will tell begin to download dependencies. and >>>> start to download criu from github repository <<<< will tell begin to download from github. Then we can know which step stack.
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
ci test alway hang on installing criu things some times ago, add
log to make sure who blocked the ci, see #2298.
Signed-off-by: Ace-Tang [email protected]
Ⅰ. Describe what this PR did
Ⅱ. Does this pull request fix one issue?
Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗?)
no need.
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews