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

Add Dockerfile to run service in a container #76

Merged

Conversation

proffalken
Copy link
Contributor

Summary

Add a Dockerfile to launch the application within a container.

NOTE: At the moment, the focalboard version is hard-coded and there is
a bug that means the download is not actually a .tar.gz file, but a
.tgz.gz file inside a .zip file.

This also assumes the use of SQLite as the database rather than PgSQL.

Ticket Link

Fixes mattermost/mattermost#75

Fixes mattermost-community#75

NOTE: At the moment, the focalboard version is hard-coded and there is
a bug that means the download is not actually a .tar.gz file, but a
.tgz.gz file inside a .zip file.
@laundmo
Copy link

laundmo commented Mar 17, 2021

Why does this do a full system upgrade in the container? Doesn't that bloat up the container size extremely?

@metanerd
Copy link
Contributor

/check-cla

@metanerd
Copy link
Contributor

Hi @proffalken ! Thank you for your PR! Sorry about the spam to set up the check-cla command from me.

@proffalken
Copy link
Contributor Author

Why does this do a full system upgrade in the container? Doesn't that bloat up the container size extremely?

Nope, it updates a single package at the moment and brings it inline with the latest security releases.

If I were installing Ubuntu-Desktop or similar then yes, that would absolutely add bloat, but this purely ensures that we are running the latest versions of any package that is already installed to ensure we're patched and up to date. At most, I'd expect it to add a couple of Kb to the image.

@proffalken
Copy link
Contributor Author

Hi @proffalken ! Thank you for your PR! Sorry about the spam to set up the check-cla command from me.

@metanerd No worries, is there a link to the CLA somewhere so I can sign it? I'm more than happy to do so!

@metanerd
Copy link
Contributor

Glad to hear that @proffalken ! The CLA form can be found here: https://mattermost.org/mattermost-contributor-agreement/

@proffalken
Copy link
Contributor Author

Glad to hear that @proffalken ! The CLA form can be found here: https://mattermost.org/mattermost-contributor-agreement/

Great! All signed @metanerd - looking forward to getting involved!

@metanerd
Copy link
Contributor

/check-cla

Dockerfile Outdated
RUN apt upgrade -y
RUN apt install -y wget tar gzip unzip file
RUN wget https://releases.mattermost.com/focalboard/0.5.0/focalboard-server-linux-amd64.tar.gz
RUn unzip -o focalboard-server-linux-amd64.tar.gz

Choose a reason for hiding this comment

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

Does this RUn typo throw a syntax error?

Also, really this should be one run command so the end result isnt a bunch of unnecessary layers

FROM ubuntu:latest

RUN apt update \
    && apt install -y wget tar gzip unzip file \
    && wget https://releases.mattermost.com/focalboard/0.5.0/focalboard-server-linux-amd64.tar.gz \
    && tar -xvzf focalboard-server-linux-amd64.tar.gz \
    && mv focalboard /opt \
    && rm -f focalboard-server-linux-amd64.tar.gz # cleanup the unnecessary zip to reduce end container size

EXPOSE 8000

WORKDIR /opt/focalboard

CMD /opt/focalboard/bin/focalboard-server

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good spot on the typo, but no, it doesn't throw an error.

As far as the "single command" is concerned, I'd be happy to condense it down to 2 commands - one for the OS and one for Focalboard? Seems like that way we only update the OS layer if it changes, regardless of the application version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jasonraimondi - I've split it out into two RUN statements instead, would appreciate your feedback?

@jespino jespino self-requested a review March 17, 2021 22:38
slashwq pushed a commit to slashwq/docker-focalboard that referenced this pull request Mar 17, 2021
@creekorful
Copy link

This is really great! Can't wait to try it with Docker 😄

Copy link
Contributor

@jespino jespino left a comment

Choose a reason for hiding this comment

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

Thanks @proffalken! this is awesome. I clearly see adding changes later to support environment variables to configure Postgres instead of SQLite. And clearly we have to fix the "zip/tgz" problem, but we can do it later.

@jespino jespino merged commit 33aba94 into mattermost-community:main Mar 18, 2021
@proffalken
Copy link
Contributor Author

@jespino - thanks for approving.

FWIW, I logged #77 to register the download format, and yes, totally agree that being able to mount the config file in the container on the appropriate path/set versions from ENV vars etc, is needed! :)

@jespino
Copy link
Contributor

jespino commented Mar 18, 2021

Thanks! This is a great first iteration, now we have something to improve and make it more configurable.

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.

6 participants