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

WIP: Docker container #22

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

Conversation

TheMBeat
Copy link

No description provided.

@TheMBeat TheMBeat changed the title Docker container WIP: Docker container Jan 27, 2024
@TheMBeat TheMBeat marked this pull request as draft January 31, 2024 17:38
@TheMBeat TheMBeat marked this pull request as ready for review February 8, 2024 20:23
Copy link
Owner

@doodlezucc doodlezucc left a comment

Choose a reason for hiding this comment

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

Thank you so much for the PR, overall it looks pretty good! I just added a few review comments. Do you think it's possible to move all Docker related files into a ./docker folder? That would make the repo look more organized in my opinion.

password: ${{ secrets.GITHUB_TOKEN }}

- name: Dev Release
if: startsWith(github.ref, 'refs/heads/') # Just the branches
Copy link
Owner

Choose a reason for hiding this comment

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

GitHub also provides github.ref_type, which is either tag or branch, so I'd recommend the following:

if: github.ref_type == 'branch'

- name: Set environment
run: |
VERSION="$(cat .VERSION)"
MAJORVERSION="$(cat .VERSION | cut -c 1-2 | sed -r 's#^(.{0})#\1latest-#')"
Copy link
Owner

Choose a reason for hiding this comment

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

MAJORVERSION is not used as far as I can tell, could we remove this line?

Comment on lines +27 to +28
export TAG="${GITHUB_REF#'refs/tags/'}"
echo "GH_TAG=$TAG" >> $GITHUB_ENV
Copy link
Owner

Choose a reason for hiding this comment

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

Prefer using GITHUB_REF_NAME here, which will resolve to either master on the master branch, or to the tag name when pushing a tag.

Comment on lines +5 to +6
org.opencontainers.image.documentation="https://github.com/buanet/ioBroker.docker#readme" \
org.opencontainers.image.authors="Marcel A." \
Copy link
Owner

Choose a reason for hiding this comment

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

We should update these lines to fit Dungeon Club (you're welcome to include your name in the authors field of the Dockerfile)

if [[ -f /opt/.docker_config/.first_run ]]; then
echo "----- Download and Unzip Ambience Music -----"

wget --no-check-certificate -O ../app/ambience/music-bundle.zip "https://evi.nl.tab.digital/s/y2w7b7e7ztPRYra/download" \
Copy link
Owner

Choose a reason for hiding this comment

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

I've created a Nextcloud account with the music bundle available to download, we can now use the following link: https://evi.nl.tab.digital/s/cKjTZ7reBkGJgHG

environment:
- ENABLE_MUSIC_PLAYER=false
volumes:
- ./DungeonClub_Data:/app
Copy link
Owner

Choose a reason for hiding this comment

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

Regarding the folder name, I'd suggest docker/dungeonclub_data in all lowercase.

Comment on lines +88 to +102
```yml
version 2.1

services:
dungeonclub:
image: ghcr.io/doodlezucc/dungeonclub:latest
container_name: dungeonclub
ports:
- 7070:7070
restart: always
environment:
- ENABLE_MUSIC_PLAYER=false
volumes:
- <path-to-data>:/app
```
Copy link
Owner

Choose a reason for hiding this comment

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

Should we link to the docker-compose.yml file instead? That way there would be a single point to view and edit the compose file.

version: "2.1"
services:
dungeonclub:
image: dungeonclub:latest
Copy link
Owner

Choose a reason for hiding this comment

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

I don't know Docker that well, so forgive me if this is a silly question. By putting dungeonclub:latest here, would the compose file search Docker Hub for a repository named dungeonclub with the tag latest? I saw you using ghcr.io/doodlezucc/dungeonclub:latest as an image in the readme file, which I guess would point to the GitHub package.

# Prepare .docker_config
&& mkdir /opt/.docker_config \
&& echo "${VERSION}" > /opt/.docker_config/.thisisdocker \
&& echo "true" > /opt/.docker_config/.first_run
Copy link
Owner

Choose a reason for hiding this comment

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

I can't seem to find a point where the .first_run file gets deleted as of now, or maybe I missed it. Does first_run get set to false?


# TODO: refactor this to not repeat all the code in the Dev Release step
- name: Version Release
if: startsWith(github.ref, 'refs/tags/') # Just the tags
Copy link
Owner

Choose a reason for hiding this comment

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

The opposite of the upper condition would be:

if: github.ref_type == 'tag'

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.

2 participants