-
Notifications
You must be signed in to change notification settings - Fork 2
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
Feature java8 + Sane java defaults for Exhibitor and Zookeeper #3
base: master
Are you sure you want to change the base?
Conversation
Dockerfile
Outdated
|
||
# Use one step so we can remove intermediate dependencies and minimize size | ||
RUN \ | ||
RUN set -ex \ |
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.
Is this something we would want everywhere, or just helpful here to give us more insight in this case?
I do like the addition of 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.
Actually this was mainly for my sanity during debugging / environment vars being set, I can nuke it now if need be. Will only run during build, not during runtime ...
|
||
EXPOSE 2181 | ||
|
||
ENTRYPOINT ["zk-smoketest-master/zk-latencies.py"] |
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.
No newline
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.
fixed
# Set initial Java heap size | ||
ZK_JAVA_OPTS="$ZK_JAVA_OPTS $ZK_JAVA_INITIAL_HEAP_SIZE" | ||
# Set maximum Java heap size | ||
ZK_JAVA_OPTS="$ZK_JAVA_OPTS $ZK_JAVA_MAX_HEAP_SIZE" |
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.
Thank you for the comments.
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.
only way we'll ever remember, SO MANY OF THEM. @mannytoledo
|
||
# Install ZK | ||
&& curl -Lo /tmp/zookeeper.tgz $ZK_RELEASE \ | ||
&& wget -O /tmp/zookeeper.tgz "${ZK_RELEASE}" \ |
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.
curious why we moved to using wget
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.
Good question, for some reason, this curl -Lo was not working for me, my guess is that the version of curl that use to work fine on debian:jessie, doesn't work on xenial ... maybe because they're several versions different between each other, maybe because of how the flags used to compile it in general are different? not 100% sure, so I got fed up w/ it and used wget instead :) @kangman
@@ -18,7 +22,51 @@ HTTP_PROXY="" | |||
: ${HTTP_PROXY_PORT:=""} | |||
: ${HTTP_PROXY_USERNAME:=""} | |||
: ${HTTP_PROXY_PASSWORD:=""} | |||
: ${ZK_JAVA_INITIAL_HEAP_SIZE:=$DEFAULT_ZK_JAVA_INITIAL_HEAP_SIZE} |
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.
Can you remind me, what this variable instantiation comes from?
: ${SOMETHING:=$SOMETHING_ELSE}
not bash, right?
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.
@digitalyuki from man bash
:
${parameter:=word}
Assign Default Values. If parameter is unset or null, the expansion of word is assigned to parameter. The value of parameter is then substituted.
Positional parameters and special parameters may not be assigned to in this way.
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.
mentioned IRL, bash-ism, @ko-be put definition above so thank you ^
Dockerfile
Outdated
# Append "+" to ensure the package doesn't get purged | ||
BUILD_DEPS="curl maven openjdk-7-jdk+" \ | ||
DEBIAN_FRONTEND="noninteractive" | ||
BUILD_DEPS="oracle-java8-installer oracle-java8-set-default maven" \ |
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.
We may need to stick to openjdk if we are pushing up to a public repo @bossjones
Dockerfile
Outdated
DEBIAN_FRONTEND=noninteractive | ||
|
||
# Workaround for bug: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=807948 | ||
RUN chmod 0777 /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.
mode 1777 is better than 0777, though not sure if 0777 is required for this particular issue.
Linked to: https://github.com/adobe-community/issues-ops/issues/1980
Features