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 basic Dockerfile #650

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Prof-Bloodstone
Copy link

This pull request...

  • Fixes a bug
  • Introduces a new feature
  • Improves an existing feature
  • Boosts code quality or performance

Description

This PR adds a very basic Dockerfile. Packaged it for myself, but someone else might also find it useful.
It uses multi-stage builds, greatly decreasing the size of the final image. If you also want to provide docker image downloads, both docker-hub and quay.io has automatic builders so there isn't need to configure much.
I've decided to put the jar in the rootfs because I never really know of a better UNIXy place. Open to suggestions.

Copy link
Contributor

@Sanduhr32 Sanduhr32 left a comment

Choose a reason for hiding this comment

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

  1. minimum required java version is 8 (also openjdk 11 has ssl issues on some systems)
  2. this builds src, which is kinda unsupported
  3. probably doesn't need to build src each time
  4. Edit: /usr/local/bin/JMusicBot.jar might be a good place or in ~/bin/ or ~/.local/bin/

@Prof-Bloodstone
Copy link
Author

  1. I chose Java 11 because it's the latest LTS release. There's no worry about it not working on some systems because, well, containers. Unless I just didn't find the case where it breaks yet. Java 11 has plenty of runtime improvements like GC, although I don't think Music Bot creates enough allocations to worry about it.
  2. I'd rather avoid copying the compiled jar from the host - this limits amount of uses for the Dockerfile itself.
  3. I tried to cache dependencies etc. I don't have any ideas on further limiting when the source is rebuilt.
  4. Thanks a lot! I also noticed the bot creates more data than I expected, so in my deployment I've moved everything.

Also probably should switch to some random, non-root user in the Dockerfile.

@Sanduhr32
Copy link
Contributor

1. I chose Java 11 because it's the latest LTS release. There's no worry about it not working on some systems because, well, containers.

no its not the latest LTS release but whatever.

3. I tried to cache dependencies etc. I don't have any ideas on further limiting when the source is rebuilt.

maybe check if the jar exists in your workdir, if not run maven for building the jar, else skip to executing the jar.

  1. has been edited

@Prof-Bloodstone
Copy link
Author

  1. What is the latest LTS then? Afaik next one is Java 17 scheduled for middle of this year, but currently it's Java 11.

  2. Sadly it's not possible to check if built jar exists. What I could do is get rid of the builder all together and only download the jar from GH releases. Downside is that it only works for releases (can't use snapshots) and needs to be updated with each release, since GH doesn't have "latest" endpoint.

  3. Good idea. I'm thinking about putting the jar in /usr/... and having workspace/volume set to /data/, though might go with /var/jmusicbot/ dir.

Thanks for all the feedback!

@Sanduhr32
Copy link
Contributor

Sanduhr32 commented Mar 30, 2021

  1. Yeah well docker can check if files and or dirs exist. Maybe learn some more docker.
RUN if [[ -f /path/file ]]; then blah; else foo; fi

@Prof-Bloodstone
Copy link
Author

Well, I use docker for a living for years now. Would love to be pointed to any sort of way to branch Dockerfile.

See for example:
https://redgreenrepeat.com/2018/04/13/how-to-conditionally-copy-file-in-dockerfile/
https://stackoverflow.com/questions/31528384/conditional-copy-add-in-dockerfile

It makes a lot of sense - such approach makes images not reproducible.

The only possibility is to whitelist "target" directory in dockerignore and copy both it and the source together, then in RUN directive in bash, decide if it's needed to run maven. This has many issues - what if target directory has old binary? Also we loose all of the caching that Docker provides - any smallest change to target dir, that doesn't result in appropriate jar being there, will invalidate the caches.

Could you please point me to any documentation in regards to this? I'd love to learn about it.

@fitzychan
Copy link

Is there an update on this? This seems to be a pretty sensible docker file to me.

To throw my 2 cents into it...

1.minimum required java version is 8 (also openjdk 11 has ssl issues on some systems)

2.this builds src, which is kinda unsupported
<Opinion on this is also under 3>

3.probably doesn't need to build src each time
<Generally you want the Docker file to build from source. This ensures that whoever is running the container is getting the exact same docker container as everyone else. By putting branching logic in the docker file is generally a bad idea. You want to treat each docker container as a fresh system, and on each fresh system you need to reinstall and rebuild... Unless you are publishing build artifacts somewhere for people to just pull.>

Of the other docker file suggestions this one seems good. I dont know why its not been merged yet.

@jagrosh
Copy link
Owner

jagrosh commented Oct 11, 2021

In general, adding a supported docker file is on-hold until this script is rewritten.

@fitzychan
Copy link

What specially are you looking for? The same functionality written in java?

@jagrosh
Copy link
Owner

jagrosh commented Oct 11, 2021

No, the script already works correctly, it just isn't ideal for platforms like docker yet because it uses values set within the script instead of using flags. TLDR the script should support --no-download, --no-loop, --background and -- ... (pass anything after -- as jvm flags)

An ideal docker file would just download and run the script

@Prof-Bloodstone
Copy link
Author

@fitzychan

  1. Yes, but it did work just fine on Java 11. I see no reason to stay on ancient version just for the sake of using old version. This is packaged in a docker image, so if it works for me, it'll work for you. If I was to PR it today, I'd test if it works on Java 17 and use it instead.
  2. That's literally what is being done? I believe this is unsupported for the sake of having less issues from people who might not know what they are doing, but correct me if I'm wrong. Since the same author would "maintain" it, I think it should be fine.
  3. It won't rebuild if nothing changed, so I don't see what's the issue there?

@jagrosh unfortunately I'm busy for a few more weeks, but if you'd create an issue describing what needs to be rewritten, I might take a look at it and PR it. Always a good excuse to write more bash :)
Maybe I can rewrite it.

@Prof-Bloodstone
Copy link
Author

No, the script already works correctly, it just isn't ideal for platforms like docker yet because it uses values set within the script instead of using flags. TLDR the script should support --no-download, --no-loop, --background and -- ... (pass anything after -- as jvm flags)

An ideal docker file would just download and run the script

Oh, that's simpler than I thought you might want. Should I include this in this or a separate PR?

@fitzychan
Copy link

@Prof-Bloodstone Sorry if I came off combative. My response was more directed at Sanduhr32's requests to fix the docker file when what you have is pretty standard.

@jagrosh
Copy link
Owner

jagrosh commented Oct 11, 2021

Well that's the short version, because there are still other problems I need to figure out. For example, generating the config file the first time would require some separate logic (both in the script and in the jar).

@Prof-Bloodstone
Copy link
Author

@jagrosh The base for your future changes to that script: #981
I don't see the reason why it's blocking this PR, but ok. It's better to have something than nothing :)

An ideal docker file would just download and run the script

You want dockerfile to have nothing other than just the script and dependencies, and then download them on start? That sounds bad :/

@MichailiK MichailiK linked an issue Jan 4, 2022 that may be closed by this pull request
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Consider offering an official Docker image
5 participants