-
Notifications
You must be signed in to change notification settings - Fork 757
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: remove chmod install scripts #2830
Conversation
Signed-off-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
Codecov Report
@@ Coverage Diff @@
## main #2830 +/- ##
==========================================
+ Coverage 70.08% 70.25% +0.16%
==========================================
Files 130 116 -14
Lines 10153 10190 +37
==========================================
+ Hits 7116 7159 +43
+ Misses 3037 3031 -6
|
Signed-off-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
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; I assume you've tested this?
Signed-off-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
Signed-off-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
Signed-off-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
cc @sauyon |
Signed-off-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
Signed-off-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
Signed-off-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
Signed-off-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
Co-authored-by: Sauyon Lee <2347889+sauyon@users.noreply.github.com>
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.
👍
@@ -225,6 +226,13 @@ def write_to_bento( | |||
setup_script = resolve_user_filepath(self.setup_script, build_ctx) | |||
except FileNotFoundError as e: | |||
raise InvalidArgument(f"Invalid setup_script file: {e}") | |||
if not os.access(setup_script, os.X_OK): |
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.
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 chmod
in the dockerfile are just docker related, we want to have a executable check here so that users make sure the script is runnable.
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.
@aarnphm it's not necessary, right? since we only run that script in docker?
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 the intention for this is to check whether a script is executable or not. But that evolve using a library like python-magic. I think we end up just doing this as a first pass.
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'm not entirely sure what we want to do here. But the check is indeed not required.
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.
🤔 We can remove this if this means less friction for user.
Signed-off-by: Aaron Pham 29749331+aarnphm@users.noreply.github.com
What does this PR address?
install.sh
and all related scripts generated by BentoML should use bash to runinstead of
chmod +x
.Usually
chmod +x
are not recommended for container application.Before submitting:
guide on how to create a pull request.
make format
andmake lint
script have passed (instructions)?those accordingly? Here are documentation guidelines and tips on writting docs.
Who can help review?
cc @sauyon