Skip to content
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

Merged
merged 4 commits into from
Sep 11, 2018
Merged

Conversation

jglick
Copy link
Member

@jglick jglick commented Aug 22, 2018

By defaulting everything except the image argument to reasonable values. Should make it easier to use particularly from Evergreen. @batmat @ndeloof

Copy link
Member

@rtyler rtyler left a 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";
Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member Author

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.

@francoisferrand
Copy link
Contributor

This seems a great improvement! Can someone please merge, and hopefully release?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants