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

feat: add docker-compose stack #1471

Merged
merged 3 commits into from Sep 14, 2022
Merged

Conversation

paddatrapper
Copy link
Contributor

@paddatrapper paddatrapper commented Jan 3, 2022

Description

Adds Dockerfiles and documentation around them

  • Shared
  • API client
  • Analyzer
  • API
  • Legacy
  • Worker
  • Playout
  • Liquidsoap
  • Documentation
  • CI push to Docker image repos

Fixes: #949
Fixes: #551
Fixes: #624

This is a new feature: yes

Do the changes in this PR implement a new feature?

I have updated the documentation to reflect these changes: TODO. Needs deployment and dev notes

Testing Notes

What I did:

Built and tested Docker images

How you can replicate my testing:

Follow the Docker build documentation (TODO)

@paddatrapper paddatrapper marked this pull request as draft January 3, 2022 07:44
Copy link
Contributor

@jooola jooola left a comment

Choose a reason for hiding this comment

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

Hey, some early review hope you don't mind.

Awesome that you started doing this, it was also my intent you beat me to it.

I left a few comments. And maybe we should pin the python release version as well.

And I would not recommend using bullseye, I mean we need to move forward and fix the codebase, but I reverted my migration to bullseye in production as I was getting way too much bugs.

analyzer/Dockerfile Outdated Show resolved Hide resolved
analyzer/Dockerfile Outdated Show resolved Hide resolved
analyzer/Dockerfile Outdated Show resolved Hide resolved
@jooola jooola changed the title Feat: add Dockerfiles for applications feat: add Dockerfiles for applications Jan 4, 2022
@paddatrapper paddatrapper force-pushed the feature/docker branch 2 times, most recently from d5dc692 to 825bc36 Compare January 4, 2022 12:25
@paddatrapper paddatrapper force-pushed the feature/docker branch 3 times, most recently from 825bc36 to 6971ebc Compare January 12, 2022 08:40
@paddatrapper
Copy link
Contributor Author

paddatrapper commented Jan 12, 2022

Building individual docker images is tricky without #1504 because the build context is each app. So inter-dependencies (e.g. on api_client) are not supported. They can only be installed from the web and not from the local repo, making pypi the logical choice. It does mean that changes to shared/api_client require a version bump before the changes can be used in any application. This is because building will always use the Pypi version, so new changes need pushing there before being used.

For example, this blocks building a Docker image for playout, as it requires the api_client package to be installed to run

@jooola
Copy link
Contributor

jooola commented Jan 12, 2022

I would recommend importing a wheel file from a build stage over fetching packages from pypi.

I used to release some docker images like this, but by the time the pip package was available on pypi, the published docker image was using the previous version. Also this would not allow use to build images that haven't been published yet. For instance having a image from master for the testing/demo feature would be infeasible.

What I propose is to create a api_client and shared dockerfile only with a build stage, run those before we build the apps, and import the wheels from there.

May I push some ideas to this branch ?

@jooola
Copy link
Contributor

jooola commented Jan 12, 2022

Pushed a extra commit with some idea, feel free to prune the commit if you don't want it.

analyzer/Dockerfile.jooola Outdated Show resolved Hide resolved
analyzer/Dockerfile.jooola Outdated Show resolved Hide resolved
@paddatrapper

This comment has been minimized.

@jooola
Copy link
Contributor

jooola commented Jan 22, 2022

I rebased this branch.

@jooola
Copy link
Contributor

jooola commented Feb 25, 2022

Rebased the branch onto main.

@paddatrapper
Copy link
Contributor Author

@jooola please make autogenerating requirements a seperate PR

@jooola
Copy link
Contributor

jooola commented Mar 4, 2022

Rebased on the main branch.

@paddatrapper Please tell if I should have my own branch if I am messing with your work.

@paddatrapper
Copy link
Contributor Author

No, it's fine. I want to go through and clean this up hopefully this week so it is ready for testing

@paddatrapper
Copy link
Contributor Author

Where do we want to publish docker images? GitHub container repo and Docker Hub?

@jooola
Copy link
Contributor

jooola commented Apr 11, 2022

I lean towards ghcr

@jooola jooola force-pushed the feature/docker branch 5 times, most recently from 1d4e673 to 27969a0 Compare September 8, 2022 22:02
@jooola
Copy link
Contributor

jooola commented Sep 9, 2022

So I've been playing and developing with this for the past days, and it is quite pleasing to use ! Everything is working as expected, and a "production" demo is available in docker/example (since no image are published yet, you have to build the images first and override the LIBRETIME_VERSION=main to run them).

This is obviously not bullet proof yet but haven't had any bug so far. This can be considered as the work in progress I was referring some days ago, as we will have to fix bugs, improve the documentation and maybe change installation procedures.

With regards to inter container dependencies, I am not yet sure if we want to wait for others services apart from the database and maybe rabbitmq. For now we have the dependencies set, maybe I'll remove them in the future if it improves the startup time, and they are properly handled.

With that said, I think we can review merge this !

@jooola jooola force-pushed the feature/docker branch 2 times, most recently from 1e4d619 to 2787565 Compare September 9, 2022 19:39
@paddatrapper
Copy link
Contributor Author

Awesome! Thanks for all the hard work. I'll try test and review on Monday

@jooola
Copy link
Contributor

jooola commented Sep 12, 2022

Rebased onto the main branch.

@paddatrapper
Copy link
Contributor Author

/website-preview

@github-actions
Copy link

github-actions bot commented Sep 14, 2022

🚀 Website preview deployment succeeded!

Website preview: https://libretime.github.io/pr-1471/
New docs preview: https://libretime.github.io/pr-1471/docs/next/
Workflow: https://github.com/libretime/libretime/actions/runs/3051410854

@paddatrapper
Copy link
Contributor Author

paddatrapper commented Sep 14, 2022

I'm getting the following error running from the root of the project:

$ docker-compose run --rm api libretime-api migrate
ERROR: The Compose file './docker-compose.override.yml' is invalid because:
services.api.environment.LIBRETIME_DEBUG contains true, which is an invalid type, it should be a string, number, or a null

@jooola
Copy link
Contributor

jooola commented Sep 14, 2022

I'm getting the following error running from the root of the project:

$ docker-compose run --rm api libretime-api migrate
ERROR: The Compose file './docker-compose.override.yml' is invalid because:
services.api.environment.LIBRETIME_DEBUG contains true, which is an invalid type, it should be a string, number, or a null

Which version of docker-compose are you using ?

@paddatrapper
Copy link
Contributor Author

Fixed by quoting the variable. Running docker-compose 1.29.2

@jooola
Copy link
Contributor

jooola commented Sep 14, 2022

Fixed by quoting the variable. Running docker-compose 1.29.2

Ok makes sense, I've only been working with docker-compose 2.x.

docs/admin-manual/setup/install.md Outdated Show resolved Hide resolved
docs/admin-manual/setup/install.md Outdated Show resolved Hide resolved
@jooola
Copy link
Contributor

jooola commented Sep 14, 2022

/website-preview

@paddatrapper
Copy link
Contributor Author

LGTM - tested and seems to work for me now. I'm excited to try this within k8s once we have built images and see how things go

@jooola
Copy link
Contributor

jooola commented Sep 14, 2022

LGTM - tested and seems to work for me now. I'm excited to try this within k8s once we have built images and see how things go

Awesome ! As we discussed earlier, I expect this to be somewhat of a work in progress. I hope that the feedback from the community will help use iron out the docker setup.

I'll merge in that case.

paddatrapper and others added 3 commits September 14, 2022 11:02
- build container with multi-stage Dockerfile
- change api listen port to 9001
@jooola
Copy link
Contributor

jooola commented Sep 14, 2022

Oh but now that you mention it, should we publish a image for the main branch ? Or only tagged release ? I only considered the tagged releases for now.

EDIT; merging, this can be tweaking in another PR

@jooola jooola merged commit 8ef82d7 into libretime:main Sep 14, 2022
@paddatrapper paddatrapper deleted the feature/docker branch September 14, 2022 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants