Allow launcher in nodejs #1854
Allow launcher in nodejs #1854
Conversation
martaver
requested review from
alexeagle,
gravypod,
pcj and
smukherj1
as code owners
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
martaver
force-pushed
the
allow_launcher_in_nodejs
branch
from
5121300
to
3b31a8c
Compare
add launcher and launcher_args attrs to nodejs_image, and pass them to app_layer update docs with note about launcher and launcher_args for nodejs_image add run-bazel-in-docker.sh allowing non-linux platforms to execute repo build and tests add .idea to .gitignore Add test for using nodejs_image with launcher arg
martaver
force-pushed
the
allow_launcher_in_nodejs
branch
from
3b31a8c
to
d757dc2
Compare
There was a problem hiding this comment.
This is awesome thanks!
You asked on slack about whether this should cover other languages, I don't really know...
Before we merge this, I have one question though. Would it be hard for a user to just skip using the convenient language rules like nodejs_image and just make their own container_image in order to get access to these attributes? maybe the use case is narrow enough that omitting them from the easy-to-use language wrappers is actually a feature to make things simpler for common users.
run-bazel-in-docker.sh
Outdated
| @@ -0,0 +1,11 @@ | |||
| mkdir -p /tmp/build_output/ | |||
There was a problem hiding this comment.
did you mean to check in this file? I guess it's for testing something. If so, maybe add some comments here about what it's used for and how to use it
There was a problem hiding this comment.
Added some docs... I created this script because the repo's tests fail on non-linux systems (I'm on MacOS):
tests/container/nodejs/nodejs_image_custom_binary_test.image: line 208: /usr/bin/docker: No such file or directory
|
re: rolling your own I would argue that launchers are common (roughly 50% of the images I deal with have them) and a minor enough implementation detail that it shouldn't warrant having to roll a custom At the end of the day, there are basically only two ways to customise a This is also why I suspect it'd be good form to adder |
|
I tried to rebase this for you to land it, but I think you disabled the checkbox "allow edits from maintainers" - could you rebase yourself or check the box? |
|
Mmmmm done... looks like Also it's weird, I don't see the 'allow edits from maintainers' option. I found it in docs, but it's just not there in the PR. What did you think of exposing the launcher args for each language image btw? |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: #1802
"entrypoint / launcher in nodejs_image isn't respected "
What is the new behavior?
The
launcherandlauncher_argsattributes are now accepted bynodejs_imageand passed toapp_layer.Does this PR introduce a breaking change?
Other information
This feature isn't really node specific. If maintainers agree, then I would update all the language images that use
app_layerto acceptlauncherandlauncher_args. Then I would also remove the documentation change fromnodejs_imageand instead make a general note that all the language images support the attributes.