-
Notifications
You must be signed in to change notification settings - Fork 322
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
fix: improve onebox work dirs #1899
Conversation
auula
commented
May 27, 2022
•
edited
Loading
edited
- What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
- fix scripts: improve onebox script #1328
- What is the current behavior? (You can also link to an open issue here)
- The log output by the onebox script is not in the unified directory.
- What is the new behavior (if this is a feature change)?
- The subdirectory can be set through the environment variable ONEBOX_WORKDIR.
- Log unified directory.
onebox/start_onebox.sh
Outdated
|
||
IP=127.0.0.1 | ||
|
||
# work unified file directory | ||
if [ ! -n "$workspace" ]; 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.
pls resolve shellcheck errors
onebox/start_onebox.sh
Outdated
|
||
IP=127.0.0.1 | ||
|
||
# work unified file directory | ||
if [ ! -n "$workspace" ]; 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.
I recommend workspace
default to a subdirectory inside onebox/
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 recommend
workspace
default to a subdirectory insideonebox/
review code :760ff33
onebox/start_onebox.sh
Outdated
# default work directory | ||
workspace="workspace" | ||
rm -rf $workspace/logs/ | ||
mkdir -p $workspace/logs |
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.
should mkdir for other workspace
too
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.
should mkdir for other
workspace
too
review code :760ff33
Codecov Report
@@ Coverage Diff @@
## main #1899 +/- ##
============================================
+ Coverage 75.69% 75.72% +0.03%
Complexity 347 347
============================================
Files 613 613
Lines 117018 117018
Branches 1024 1024
============================================
+ Hits 88574 88616 +42
+ Misses 28235 28193 -42
Partials 209 209
Continue to review full report at Codecov.
|
onebox/start_onebox.sh
Outdated
rm -rf logs/ | ||
mkdir -p logs | ||
# rm -rf logs/ | ||
# mkdir -p logs |
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.
remove the comments, line 22-23
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.
remove the comments, line 22-23
remove comments: c547f44
onebox/start_onebox.sh
Outdated
# mkdir -p logs | ||
|
||
# The subdirectory can be set through the environment variable ONEBOX_WORK | ||
workspace=$ONEBOX_WORK |
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.
You can use WORKSPACE=${ONEBOX_WORKDIR:-onebox/workspace}
No need to do if ! -n "$workspace"
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.
You can use
WORKSPACE=${ONEBOX_WORKDIR:-onebox/workspace}
No need to doif ! -n "$workspace"
review code :760ff33