-
Notifications
You must be signed in to change notification settings - Fork 310
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
Execute start hook if defined in cloudbin image #1552
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1552 +/- ##
==========================================
- Coverage 36.58% 36.55% -0.03%
==========================================
Files 88 88
Lines 8460 8466 +6
==========================================
Hits 3095 3095
- Misses 4997 5003 +6
Partials 368 368
Continue to review full report at Codecov.
|
images/bin/start.sh
Outdated
@@ -14,6 +14,10 @@ if [ -d "/var/okteto/cloudbin" ]; then | |||
fi | |||
fi | |||
|
|||
if [ -d "/var/okteto/hooks/start.sh" ]; then | |||
/var/okteto/hooks/start.sh |
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.
start.sh might not be executable. I think it's easier to just run sh /var/okteto/hooks/start.sh
or source it?
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 this command fails, will up fail?
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 didn't use sh
to avoid a level of indirection, but I guess it is fine.
About the script failing, I would fail okteto up
, it would be an unexpected behavior otherwise. If the user wants to ignore errors, she can handle them on their script
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.
Cool, let's just document that we expect start to be an executable in that case.
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.
@rberrelleza also, we are not showing the output, the user will need to do a kubectl logs podname
.
I am expecting the hook to be fast, if it does things like installations, I would run it via exec (as we do for the cleanup) so we can provide interactive output
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 moved the logic to run the script client-side when needed
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.
What motivated the change? The log?
Also, is the command running after all the files are synchronized or before all that?
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.
Yes, it is to have more control over the logs and errors. It is executed right after the synchronization is finished
Signed-off-by: Pablo Chico de Guzman <pchico83@gmail.com>
Signed-off-by: Pablo Chico de Guzman pchico83@gmail.com
This makes it possible to run a start.sh hook on every dev container in the cluster