potiuk commented on PR #24825: URL: https://github.com/apache/airflow/pull/24825#issuecomment-1173825804
This is really cool. Before I get into details of the image, I think what we need here is some kind of testing to make sure the Dockerfile works and that it can be used - connected with a documentation on how it can be used. Eventually - when such code is contributed, we need to keep it "working" and our "users" need to know that they can use it and how. We have a number of such "checks" and verifications in our repo for our "debian" base image and while it is certainly not necessary to get to the level of details and tests we have there, there needs to be some way that: a) tests are run in the CI so that we are sure the neither CentOS nor any of the dependenies broke the image building process b) users will discover they can use that image - either by building their own custom image or extend our image that we publish in dockerhub c) users should know how they can customize to their needs (for example choose different airflow version, different python vesion, different centos base image (maybe). This might be just a doc or possibly support in the form of `--build-args` (same approach we have in our debian dockerfile. d) we would publish the image in our DockerHub account Not all of that needs to be impemented all at once in single PR but I think we should agree here what is the "target" we want to achieve (in follow-up PR) and what "level of a) b) c) d)" above should be in the first PR My first thought for this PR: a) we should at least build the image in CI. using latest released airflow version. b) documentation should be added to https://airflow.apache.org/docs/docker-stack/index.html to let users know we give them a possibility to build CentoOS-based image using our Dockerfile.centos (and explain them how). Ideally examples of building it should be connected to tests ans should be included in our docs - we are currently doing that here - all our examples here are embedded from our scripts, and those scripts are actually executed in CI https://airflow.apache.org/docs/docker-stack/build.html#adding-new-apt-package . It does not have to be yet as comprehensive as our debian images, it could be one or two examples, but they should be there and guide the users. c) I think adding build-args to support airflow version and python version should be good to start with - then the examples from b) could run a few combinations of buidls (and actually build them on CI). We should run it for all python versions that we support (3.7 - 3.10) or - if there are any reasons some of those are not supported we should mention why d) I think that woudl be nice to have, but this can be done way later - when we will publish the "buildable" centos image we can also make it automatically published in Airflow Repo. This would likely require some discussion and likely lazy-consensus on Airlfow devlist so I am happy to make it happen. Once (or during) a) b) c) are adressed - I can make a detailed review then. Seeing the CI builds that pass the builds would be great mark that it is ready to review. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
