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

Batch of updates for the image #12

Merged
merged 14 commits into from
Mar 30, 2017
Merged

Batch of updates for the image #12

merged 14 commits into from
Mar 30, 2017

Conversation

dduportal
Copy link
Contributor

Hello there !

Follow up of #11 without alpine.

After some time using my own image which purpose is almost the same as this one, here is an update proposal that cover an upgrade to latest docker practises (1.13.2 at the time starting this PR), Jenkins SSH usages, and also shrinking the image to spend less disk space.

Here are the major changes:

  • Base image moved from "java" (deprecated in 2016) to "openjdk"
  • No more slave.jar (test upgraded) embeded since the Jenkins SSH agent protocol takes care of downloading and upgrading it at startup
  • Ensuring that bash is the default command line (cf. https://issues.jenkins-ci.org/browse/JENKINS-41374 )
  • Adapting Dockerfile instructions to use builds ARGS to allow specification of user and groups, uid and gid, following the official Jenkins image
  • SSH is now writing its logs to stderr, allowing docker logs to fetch the SSH output (and not written within a file)
  • Additional data volume have been added to improve agent performances (writing temp files on host FS and not on the Layered FS)
  • The default user home directory is now variabilized for better maintenance or custom build use cases
  • git is now explicitly installed (tradeoff because it generally require root rights to install, so interesting to have it here, while mercurial don't)
  • Some bash shellchecking

Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>
Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>
Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>
Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>
Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>
Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>

Cleaning

Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>
…lysis of bad keys

Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>
Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>
Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>
Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>
…nfusion when building from Windows

Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>
Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>
@oleg-nenashev
Copy link
Member

@reviewbybees

@ghost
Copy link

ghost commented Mar 15, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

Inclusion of Git requires a wider discussion. Maybe it is better to do it in a separate image (with other common tools like curl/wget & Co)

Dockerfile Outdated
RUN sed -i 's/#PasswordAuthentication yes/PasswordAuthentication no/' /etc/ssh/sshd_config
RUN apt-get update \
&& apt-get install --no-install-recommends -y \
openssh-server git \
Copy link
Member

Choose a reason for hiding this comment

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

I am not convinced Git is required in the image.
Good for demos, but Jenkins has tool installers which can install a portable Git client

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

mkdir -p "${JENKINS_AGENT_HOME}/.ssh"
echo "$1" > "${JENKINS_AGENT_HOME}/.ssh/authorized_keys"
chown -Rf jenkins:jenkins "${JENKINS_AGENT_HOME}/.ssh"
chmod 0700 -R "${JENKINS_AGENT_HOME}/.ssh"
Copy link
Member

Choose a reason for hiding this comment

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

I will follow-up in the PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure to understand this ?

@@ -1,4 +1,4 @@

#!/bin/bash
Copy link
Member

Choose a reason for hiding this comment

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

"-ex" as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, updated :)

Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>
Signed-off-by: Damien DUPORTAL <damien.duportal@gmail.com>
@dduportal
Copy link
Contributor Author

Git removed, and secured the bash helper for tests.

Not sure to get WDYM by the PM follow up (what is the PM acronym for ? )

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

🐝 and @reviewbybees done

@ndeloof ndeloof merged commit 8633848 into jenkinsci:master Mar 30, 2017
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.

3 participants