-
Notifications
You must be signed in to change notification settings - Fork 319
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
Make it easier to run dockerNode #679
Conversation
…image argument to reasonable values.
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.
🎉
@@ -36,27 +37,31 @@ | |||
|
|||
private String image; | |||
|
|||
private String remoteFs; | |||
private String remoteFs = "/tmp"; |
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.
OTOH DockerTemplate.provisionNode
seems to default this to container’s WORKDIR
and, failing that, /
. So maybe this should be left empty by default and /tmp
being the default there? I am afraid of using /
as a remoteFs
; seems bound to cause permissions problems etc.
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'd suggest that, in general, the best idea is to make the pipeline-defined nodes work in the same way that the cloud-provisioned nodes work.
IMO using the container's workdir as a default value is much better than any other value one can come up with. e.g. /tmp
could be a ramdisk or other size-limited area. /home/jenkins
is a common choice but, strictly speaking, /
is the only place one can rely on being present if there's no workdir defined.
Perhaps it might be better to fail the PipelineNodeStep if the specified container doesn't have a workdir and the user hasn't specified a remotefs?
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.
At least in the case of openjdk
, using /
as the remote FS seems to work OK (the workspace is then /workspace
owned by root
), so I think I will just let it be used—no reason to aggressively fail the step.
This seems a great improvement! Can someone please merge, and hopefully release? |
By defaulting everything except the image argument to reasonable values. Should make it easier to use particularly from Evergreen. @batmat @ndeloof