joshuabach / gnucash-web

A simple, mobile-friendly webinterface for GnuCash
GNU General Public License v3.0
118 stars 18 forks source link

Dockerization #43

Closed Freddo3000 closed 1 year ago

Freddo3000 commented 1 year ago

This pull request dockerizes the project, making it easier to spool up and host. For this a new Dockerfile is added, which I've tried to keep lightweight using Alpine. Two sample docker-compose.ymls are also provided, one running simple sqlite and the other running postgresql. To make it more production-safe, the docker version runs with the gunicorn WSGI server.

Some additional changes to the repo is that I've moved the source into a separate src directory, to simplify the docker build. I've also edited config.py to support running using only environment variables, to ease docker configuration. It also adds a workflow for publishing the docker image to https://ghcr.io, which can be previewed on my repo here.

edit:

resolves #44, as that was an issue I also noticed.

joshuabach commented 1 year ago

Hey Freddo,

thanks for the PR, Dockerisation would indeed be a good addition. But boy it is a big one ^^ It'll take some time to go through this.

A few things I have spotted already:

Superfluous files

I noticed that you commited your .idea folder, which I think is IDE-specific metadata that should not be included in the repo. It also seems that the src/ subfolder contains build files. At least some of these are listed in .gitignore, but only if they appear on the top level. Can you remove those, update .gitignore and check that there are no other files that you accidentally commited?

Commit structure

I think your commits are a bit scrambeled. I'm not a fan of squashing PRs into a single commit, especially if they do a few separate things. On the other hand, at least your "tweak docker compose" hotfix-commit could be squashed into the first commit (nitpick: which should be named "Add docker setup", or sth. similar, see subject naming conventions). Idealy, I think there should be these commits:

  1. Move files to src/ subdirectory
  2. Add support for using environment variables only for configuration
  3. Add docker setup

Number 3 could include the github action and the README update since those are both inherently part of an dependent on the dockerisation.

That would make reviewing much easier. However, at least the second part requires some git knowledge to do. If you have any more questions or need help, just say so.

Thank you!

Freddo3000 commented 1 year ago

I've removed the .idea directory and added it to .gitignore, and the accidentally committed build/dist files (I was wondering why Git wasn't detecting moved files...). As for commit structure, I was as you mentioned working on the assumption that the entire PR would be squashed and merged, so I didn't pay close attention to it. If you'd like, I could redo the changes in a new PR with a git history like you describe.

joshuabach commented 1 year ago

Without the accidentally commited files, the PR looks much more managable, so you can leave it as it is and I will squash it upon merge. I hope I can review this on Friday.

joshuabach commented 1 year ago

So functionality-wise this seems to be working great, thanks again!

Only the README-comment open then. This is not strictly a blocker for merge, but while we are at it, i would prefer getting it right.

Please note that i am unavailable from monday until the 23rd, so I can't guarantee we can get this merged before that. But it will definitly make it into the next release.

Freddo3000 commented 1 year ago

Finally sat down working on this after being swamped for a month...

joshuabach commented 1 year ago

Ok now I guess it was my turn to not respond you in a while :-)

Anyway looks good. Thank you again for your contribution and your effort of interacting with me!