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.

Sign up for GitHub

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

Jump to bottom

Dockerhub deprecation warning #181

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Dockerhub deprecation warning #181

wants to merge 7 commits into from
+117 −56

Conversation

demenech
Copy link
Member

Related to: #119

Changes

  • Added a "docker-prerun.sh" script to serve as the entry point of the docker file, that right now only echoes a deprecation warning in case the image is being pulled from Dockerhub, but also that can be extended in the future for different deprecation warnings
  • If the image is build with --build-arg IS_DOCKERHUB=true, upon running the image, the following warning can be seen:

image

I have built the docker image at the point of this PR and pushed that to Dockerhub, please try it out if you can https://hub.docker.com/repository/docker/datopian/giftless/tags/0.6.2/sha256-a7f53727881796de19c169899e7f9cb4d9e701803958855f52f8150c4d10f9b5

Future

  • If this PR is approved, I'll also push the same image with the "latest" tag and tweak the descriptions at the Dockerhub repo to flag that it's deprecated.
  • At first, I didn't want to have an "IS_DOCKERHUB" env var. Wanted something like "PRERUN_ARGS" so that the prerun script could be extended more easily, but had some difficulties with that, perhaps something to revisit later on

@demenech demenech mentioned this pull request Dec 26, 2024
Open
@rufuspollock
Copy link
Member

@athornton @vit-zikmund how does this look?

@vit-zikmund
Copy link
Collaborator

The warning seems legit, but the handling of tini is not right. My OCD also tells me not to introduce any runtime ENV var unless its being used by the main code. Also my "overengineering gate" (which is a thing I'm starting to embrace fairly recently) tells me not to introduce new generic features unless it's apparent the extensibility is worth the loss of code simplicity. Working on a followup commit that would adhere to what I described ;)

@vit-zikmund
Copy link
Collaborator

Here's the update, please @demenech have a look.
I took the liberty to use the same branch, we can revert/force push that commit if you don't like my solution. On retrospect, this move was rather presumptuous and I don't want to shadow anyone. Sorry. Next time, I'll rather use my own branch.

FYI the Dockerfile as a whole is pretty suboptiomal for build re-runs and it's necessarily bloated, containing all the project files, where only a fraction is actually needed. I'd try at some improvements, but not before this PR is done.

@vit-zikmund
chore(cleanup): uv is not a runtime requirement
02e41c0
@vit-zikmund
feat(docker): Cut docker image size down to a half
6d70618 
Using a couple best practices for saving space and build cache optimizations to get better build times and much smaller final image.

Notably:
- Python packages are now kept in a virtual environment, which is fully prepared in the builder stage, later simply copied to the same path of the final image
- Any python cache .pyc files are not part of the final image
- Git binary needed to automatically determine the package version is not part of the final image
- Many auxiliary project source files (incl. the local .git dir(!)) are also omitted from the final image
- Use RUN --mount instead of COPY where the files are needed only for the command
- Keep various COPY operations to the end of a stage not to invalidate semantically unrelated cached layers
- Remove deprecated (and misplaced) MAINTAINER directive in favor of a particular "authors" LABEL
@vit-zikmund vit-zikmund mentioned this pull request Jan 2, 2025
Merged
@vit-zikmund
Copy link
Collaborator

vit-zikmund commented Jan 2, 2025 •
edited
Loading

I eventually did a Dockerfile revamp in #183 (that's in fact targeting this PR's branch, as it's derived from it). There are some pretty neat improvements, so you might want to see that before merging it to the branch here (and then to main).

Sorry for this awful gratuitous improvement mess of mine, but I sincerely hope you'll love it in the end ;)

Happy New Year, y'all! 🎉

@demenech
Copy link
Member Author

demenech commented Jan 5, 2025

hello @vit-zikmund

Great reasoning, thanks for this awesome work! #183 LGTM, I'll wait for @athornton 's review as well and then I'll build and push to Dockerhub.

Happy New Year!

@vit-zikmund vit-zikmund mentioned this pull request Jan 8, 2025
Open
3 tasks
@vit-zikmund
Copy link
Collaborator

vit-zikmund commented Jan 9, 2025 •
edited
Loading

Hi @demenech, given the latest development in #174 (comment), this likely obsoletes the original effort in this issue. But since it now contains the fixed Dockerfile (ay-ay-ay 🙈), would it make sense to you that we remove the docker-entrypoint logic and use this PR as the vehicle for the dockerfile revamp? I might as well add the uv pip @athornton desired ;)

@demenech
Copy link
Member Author

demenech commented Jan 9, 2025

hi @vit-zikmund

Agreed. We can recover the entry point from the commit history if it's needed in the future.

@vit-zikmund
feat(docker): use uv in place of venv & pip
c7c1d9f 
This is faster and buys another 10ish MBs from the image size, likely because the uv venv is thinner.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@athornton athornton Awaiting requested review from athornton

@vit-zikmund vit-zikmund Awaiting requested review from vit-zikmund

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@demenech @rufuspollock @vit-zikmund