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

Change Dockerfile to compile python dependencies at build time. #793

Merged
merged 1 commit into from Jul 14, 2021
Merged

Change Dockerfile to compile python dependencies at build time. #793

merged 1 commit into from Jul 14, 2021

Conversation

wrecker
Copy link
Contributor

@wrecker wrecker commented Jul 12, 2021

WIP PR for #792

@zorun
Copy link
Collaborator

zorun commented Jul 12, 2021

This looks good so far, thanks. You can remove the NIGHTLY variable as it's now unused (it's the default)

I'm not convinced we want to keep the old Dockerfile around. For release builds we should probably just build it once and publish it on the dockerhub or something.

Can you also have a look at the documentation and see what needs to be updated? https://ihatemoney.readthedocs.io/en/latest/installation.html#with-docker

I see you bump Flask-SQLalchemy, why did you need that?

@wrecker
Copy link
Contributor Author

wrecker commented Jul 13, 2021

I see you bump Flask-SQLalchemy, why did you need that?

I ran into this SQLAlchemy issue that was causing the startup to fail pallets-eco/flask-sqlalchemy#910 (comment).
I will remove this change from this PR and send a separate PR for just this change.

@wrecker
Copy link
Contributor Author

wrecker commented Jul 13, 2021

Ok. Made the changes to remove the original Dockerfile and updated the Docker section of the docs.

@zorun
Copy link
Collaborator

zorun commented Jul 14, 2021

Thanks. You need to squash your commits before you rebase to master, otherwise you'll get conflicts.

Here are some more suggestions:

  • add .git/ to a .dockerignore file to avoid including 40 MB of git history for nothing
  • remove all apk packages: there's no need for them now
  • generate_hashed_password in the doc looks like a typo, it should be generate_password_hash

- Updated entrypoint.sh
- Updated docs for running with Docker
- Added .dockerignore
@wrecker
Copy link
Contributor Author

wrecker commented Jul 14, 2021

You need to squash your commits before you rebase to master, otherwise you'll get conflicts.

Done.

Here are some more suggestions:

* add `.git/` to a `.dockerignore` file to avoid including 40 MB of git history for nothing

Good tip!. Done.

* remove all apk packages: there's no need for them now

Done.

* `generate_hashed_password` in the doc looks like a typo, it should be `generate_password_hash`

Done.

@zorun
Copy link
Collaborator

zorun commented Jul 14, 2021

Excellent, thanks!

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.

None yet

2 participants