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

Improved Dockerfile: #62

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Crazy-Hopper
Copy link

  • utilized stage build which results in much smaller result image;
  • removed unneeded VOLUME declaration;
  • copied default configuration file into result image into /app/etc;
  • symlinked /app/config.toml to /app/etc/config.toml for backward
    compatibility.

- utilized stage build which results in much smaller result image;
- removed unneeded VOLUME declaration;
- copied default configuration file into result image into /app/etc;
- symlinked /app/config.toml to /app/etc/config.toml for backward
  compatibility.
@Crazy-Hopper Crazy-Hopper mentioned this pull request Oct 28, 2018
@codecov
Copy link

codecov bot commented Oct 28, 2018

Codecov Report

Merging #62 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #62   +/-   ##
=========================================
  Coverage     88.12%   88.12%           
  Complexity      208      208           
=========================================
  Files            39       39           
  Lines           522      522           
  Branches         35       35           
=========================================
  Hits            460      460           
  Misses           40       40           
  Partials         22       22

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2cd5c22...9d961e6. Read the comment docs.

@gustavkarlsson
Copy link
Owner

Thanks for contributing!

A few things:

  • Looks a lot cleaner, and I learned something about multi staged builds! But...
  • How is the resulting image smaller? I tried building with and without your changes and they both ended up at 640MB...
  • The "default configuration file" is not a valid config file in itself so it can't be used as as suggested

@igortg
Copy link

igortg commented Aug 25, 2020

How is the resulting image smaller? I tried building with and without your changes and they both ended up at 640MB...

I'd guess that most of the size is openjdk image's fault. So no point in doing multi-stage build. I'd remove it.

The "default configuration file" is not a valid config file in itself so it can't be used as as suggested

There's no way of setting a "default configuration file" that would work (unless you get a demo JIRA instance that we can point to). By adding a default config file, at least we would not get the current "/app/config.toml do not exists" error when starting the container without -v option.

The removal of the VOLUME line is the most important part of this PR, since right now it's not possible to use docker config or docker secret (which are more fit for config stuff) to set the service configuration. What about removing the multi-stage code and merge this?

And by the way, great work. Very useful integration.

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