-
Notifications
You must be signed in to change notification settings - Fork 34
Fix version number in Docker builds #933
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
base: develop
Are you sure you want to change the base?
Conversation
|
|
||
| FROM base | ||
|
|
||
| ARG EXPLORER_VERSION="" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will invalidate the docker cache from this line and onwards.
|
I want good version numbers, but I'm skeptical to the approach in this PR. What is missing for setuptools_scm to behave correctly inside the image, or what things would need to be removed from |
For setuptools_scm to operate correctly in the build, it needs to have a clean checkout of the tagged/released git repo. There's a whole bunch of things that are being ignored at the moment that exist in the repo. This is clearly not quite right, but what would you propose as a better fundamental approach? I did also consider building a wheel outside of the docker, and then installing that inside. I'm not at all happy with the current Dockerfile, I really don't like the combined development/deployment approach. If we were to keep it combined, it should be done using |
|
How about switching version numbering scheme (https://setuptools-scm.readthedocs.io/en/latest/extending/#version-number-construction) to |
3d53e0a to
26c2e7e
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #933 +/- ##
========================================
Coverage 84.23% 84.23%
========================================
Files 35 35
Lines 4199 4199
Branches 526 526
========================================
Hits 3537 3537
Misses 471 471
Partials 191 191 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
We were getting an unclean git repo inside of the Docker build which is where setuptools_scm determines the version number. This happend because our .dockerignore was excluding some directories that exist in the git repo but aren't required in the docker image. That's reasonable, but it makes it impossible to determine the version number correctly inside of the docker build. This change add a docker ARG, and determines the version number in the GitHub Actions workflow before building the image. Closes opendatacube#897
Use uvx + setuptools-scm instead of git + sed. If setuptools-scm configuration is changed we'll stay consistent.
f0e516a to
0c57ae2
Compare
| ARG ENVIRONMENT=deployment | ||
| ARG EXPLORER_VERSION="" | ||
|
|
||
| ENV SETUPTOOLS_SCM_PRETEND_VERSION=${EXPLORER_VERSION} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be required, it's supposed to be handled by line 72.
| # DOCKER STUFF | ||
| up: ## Start server using Docker | ||
| docker compose up --quiet-pull | ||
| export EXPLORER_VERSION=$$(uvx setuptools-scm) && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should have something like this earlier in the Makefile:
ifdef NO_UV
VERSION_CMD = git describe --tags | sed 's#\-g#+g#'
else
VERSION_CMD = uvx setuptools-scm
endifand then use $(VERSION_CMD) inside the parenthesis in the places that now have $$(uvx setuptools-scm). This would make it possible to build the image even for someone who doesn't have uv installed on their machine (me, as one example).
We were getting an unclean git repo inside of the Docker build which is where setuptools_scm determines the version number. This happend because our .dockerignore was excluding some directories that exist in the git repo but aren't required in the docker image.
That's reasonable, but it makes it impossible to determine the version number correctly inside of the docker build.
This change add a docker ARG, and determines the version number in the GitHub Actions workflow before building the image.
Closes #897
📚 Documentation preview 📚: https://datacube-explorer--933.org.readthedocs.build/en/933/