(Almost) Universal batch job dockerfile #613
Conversation
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## uv_at_last #613 +/- ##
===========================================
Coverage 81.07% 81.07%
===========================================
Files 130 130
Lines 5876 5876
===========================================
Hits 4764 4764
Misses 1112 1112
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| SQLAlchemy~=1.3.24 \ | ||
| tileputty~=0.2.10 | ||
|
|
||
| RUN ln -s /usr/include /usr/include/gdal |
There was a problem hiding this comment.
Why did you need to do this here and also at line 15. Probably an unneeded duplicate?
Also, can you put a comment here on why you need to do this at all? Are the gdal header files all in /usr/include/* and you also want to make them appear under /usr/include/gdal/*?
There was a problem hiding this comment.
Oops, unneeded duplicate almost certainly! As to why, yes, I think it was just that. I'll add a comment. And possibly try omitting it, in case it's changed since it was necessary.
| LABEL desc="Docker image with ALL THE THINGS for use in Batch by the GFW data API" | ||
| LABEL version="v1.1" | ||
|
|
||
| ENV TIPPECANOE_VERSION=2.72.0 |
There was a problem hiding this comment.
I assume it is intentional that you are upgrading tippecanoe so much from v1.3.1 to v2.72.0?
There was a problem hiding this comment.
Same situation as above. We were using v2.55.0: https://github.com/wri/gfw-dockerfiles/blob/master/data-api-tippecanoe.dockerfile#L5
| @@ -0,0 +1,66 @@ | |||
| FROM ghcr.io/osgeo/gdal:ubuntu-full-3.9.3 | |||
There was a problem hiding this comment.
Seems like a big jump from gdal v1.2.2 in the gdal-python.dockerfile to gdal v3.9.3? Are you sure there are no incompatibilities? I guess we just need to do a lot of testing?
There was a problem hiding this comment.
Oh, no, that's just the (somewhat arbitrary) version of the dockerfile. v1.2.1 corresponds to GDAL v3.8.5 or so. So it's not that big a version bump. See here: https://github.com/wri/gfw-dockerfiles/blob/master/data-api-gdal.dockerfile#L1
| @@ -50,7 +50,7 @@ module "batch_gdal_python_image" { | |||
| image_name = substr(lower("${local.project}-gdal_python${local.name_suffix}"), 0, 64) | |||
| root_dir = "${path.root}/../" | |||
| docker_path = "batch" | |||
| docker_filename = "gdal-python.dockerfile" | |||
| docker_filename = "universal_batch.dockerfile" | |||
There was a problem hiding this comment.
**Can you have a single 'module "universal_batch_image" at line 48, adjust the image_name at line 50, remove lines 65-81, and then adjust lines 224, 226,227 to use the same docker url based on the common image name? That way, you're only creating one docker instead of 3 during each data API deployment?
There was a problem hiding this comment.
Good point, will combine, thanks!
Pull request checklist
Please check if your PR fulfills the following requirements:
Pull request type
Please check the type of change your PR introduces:
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this introduce a breaking change?
Other information