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

Shell.DescriptorImpl.getShellOrDefault is not reliable in the presence of LauncherDecorator #95

Merged
merged 2 commits into from
Jun 5, 2019

Conversation

jglick
Copy link
Member

@jglick jglick commented Jun 4, 2019

Just because the container running the Jenkins agent has /bin/sh does not mean the container running the actual process will. For that matter, the remote callable does not even check—it just blindly returns /bin/sh on non-Windows platforms. Useless.

May conflict with #92, though that retains the shell launcher as a fallback.

See jenkinsci/kubernetes-plugin#490 for integration test.

@jglick
Copy link
Member Author

jglick commented Jun 4, 2019

Hmm, some sort of CI problem…

@jglick jglick closed this Jun 4, 2019
@jglick jglick reopened this Jun 4, 2019
Copy link
Contributor

@car-roll car-roll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I can rebase off of these changes. I don't touch that part of the code. FOr the unit tests, I have it completely refactored and can addthe slimfixture as another docker container to run the tests against.


public static final String SLIM_JAVA_LOCATION = "/usr/local/openjdk-8/bin/java";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for me it's /Library/Java/JavaVirtualMachines/jdk1.8.0_201.jdk/Contents/Home/ :)
TBH I haven't been trough all the code to be able to really understand this so maybe I'm wrong with my comment but I was a bit doh :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is about location inside openjdk:8-jre-slim, which changed recently with docker-library/openjdk#322 and was not in any of the expected locations apparently.

(docker-fixtures rebuilds images from Dockerfile on demand, and our base images are not immutable tags, so we are in the unfortunate position that CI builds sometimes fail for reasons unrelated to the patch at hand, as here.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jglick thanks for the explanation

@jglick
Copy link
Member Author

jglick commented Jun 5, 2019

rebase off of these changes

merge is better IMO.

@car-roll car-roll merged commit dac5d1b into jenkinsci:master Jun 5, 2019
@jglick jglick deleted the bourneShellElsewhereInPath branch June 6, 2019 19:31
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.

4 participants