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: Build and publish docker images #47

Merged
merged 44 commits into from
Feb 2, 2024
Merged

Conversation

6ixfalls
Copy link
Contributor

@6ixfalls 6ixfalls commented Jan 14, 2024

No Docker images are published for either the server or worker apps, although Dockerfiles are present in the repository. In the current state, this PR contains a GitHub Actions workflow to build and publish the images for the server and worker apps, as well as modify the Dockerfiles in those apps to have a correct startup command. More work is required for a Docker deployment in production, as the client application is missing a Dockerfile, but that can be either addressed in another pull request or integrated into this one - unless it's planned to create an "all in one" image which runs both the client, server, and workers, and properly reverse proxies requests to the correct backend.

To-do

  • Github Actions building
  • Docker images for all apps
  • Environment variables at runtime
  • Switch images to use an alpine base for smaller images
  • Docker deployment guide
  • Fix hacky packaging methods (some modules are manually installed)

@Shpigford
Copy link
Member

@6ixfalls Thanks for this! So, to your "all in one" point. At the end of the day, we want this to be absolutely as easy as possible for folks to get up and running. The fewer steps the better.

Does going the "all in one" route affect that one way or the other?

@6ixfalls
Copy link
Contributor Author

Does going the "all in one" route affect that one way or the other?

How I see this is that images can be published individually for each app, for example, maybe-finance/maybe-server, maybe-finance/maybe-client, and maybe-finance/maybe-worker. That approach is what this PR is mainly based on and is setup for that, (excluding maybe-client for the frontend, which can be added easily). As you mentioned, an all-in-one image would definitely be easier for users to deploy, as the entire app can be packaged into one image, and run with a single command. However, considering that maybe already has dependencies on other docker apps (eg. Redis, Postgres, etc.), I don't think an all-in-one image would be that useful, as both deployment methods would still require a docker-compose configuration, and splitting into multiple images would allow for highly available deployments, at least when using Kubernetes. Another alternative is to have 4 images, 3 for the main apps, and another "all-in-one" which combines them together into one image. Either way, an all-in-one image would need some code written or must be deployed with a reverse proxy like HAProxy, Traefik, or Nginx, as from my quick code review each app has to be deployed on separate domains, so that might not be the best way to go - a final option could be to combine all the apps so that everything can be run inside Nextjs, and that can be easily condensed into one image.

@Shpigford
Copy link
Member

@6ixfalls Gotcha. And what's the least-complicated at this moment from a speed-and-ease-of-development perspective? Or rather, what has the lowest chance of problems for developers wanting to work on this?

@6ixfalls
Copy link
Contributor Author

Gotcha. And what's the least-complicated at this moment from a speed-and-ease-of-development perspective? Or rather, what has the lowest chance of problems for developers wanting to work on this?

To be fair, most of the work in this PR is only for the GitHub Action which does the building, and the Docker images themselves are just from the original repository. In my opinion, the way maybe-finance is structured isn't ideal as it takes quite a bit of time to understand how everything is laid out, built, and deployed, but this is likely the best solution for now as it simply utilizes the existing code structure.

@6ixfalls
Copy link
Contributor Author

6ixfalls commented Jan 17, 2024

There also is a bit of work still needed for this PR before it should be merged, namely the frontend Docker image, which I'm working on now, as well as planning for how maybe-finance is released, following semver, another versioning scheme, etc - but that could very well be done at a future date.

@Shpigford
Copy link
Member

There also is a bit of work still needed for this PR before it should be merged, namely the frontend Docker image, which I'm working on now, as well as planning for how maybe-finance is released, following semver, another versioning scheme, etc - but that could very well be done at a future date.

Sounds good. Would you mind converting to Draft until its ready for review/merge?

@6ixfalls 6ixfalls marked this pull request as draft January 17, 2024 02:17
@6ixfalls
Copy link
Contributor Author

@Shpigford, the client app doesn't successfully build with some dependency issues (some dependencies aren't included in the dist package.json, e.g. @next/bundle-analyzer). I don't have much experience with nx, any ideas how to fix this or should I open a separate issue?

@6ixfalls 6ixfalls marked this pull request as ready for review January 24, 2024 05:30
@6ixfalls 6ixfalls marked this pull request as draft January 24, 2024 07:54
@6ixfalls 6ixfalls marked this pull request as ready for review January 28, 2024 02:48
@tmyracle
Copy link
Contributor

This is looking good, thanks for taking this on! Just some merge conflicts that need to be taken care of. Also, anything that needs to be updated to account for the move to pnpm that was just implemented?

@6ixfalls
Copy link
Contributor Author

A few npm commands need to be moved to pnpm, but it should be the same.

@Shpigford Shpigford merged commit efb7e62 into maybe-finance:main Feb 2, 2024
0 of 3 checks passed
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