-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
Add recipes for installing a few common tools in Docker image #13655
Conversation
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.
TBH I would not put this inside the airflow repo. We can have a separate repo imo.
Do you think this will be kept "up-to-date"?
Please let me know what you think.
@feluelle I would like it to be in one place along with the documentation for Docker image. It's not a complicated code, so I don't think there are any problems with maintaining it. These are code snippets from several images that we use in production at Polidea. These tools are required by Airflow operators, so I think, we should describe how to install these additional optional components in the image as this is not always obvious. Ideally these tools would be installed in the image, but each user may need a slightly different version and it would also add significantly to the size of the image. As for a separate repository, we agreed in the community that we would maintain a monorepo. I was wondering if this should be in the documentation for Other projects that have separate documentation about Docker have such sections in the documentation. |
I think it's good idea to add it. This is mostly documentation. however I'd add a test to build those images in CI. This way we will be sure it is up-to date. @mik-laj WDYT? |
@potiuk we have a bit overloaded our CI system, so I don't think that adding more types of tests will be a good idea, especially since building an image is not too light operation and is also crucial for the viability of our project. What do you think I'd have created a separate ticket and added these tests in the future when our CI would be a little more stable? I don't think those images would get damaged during this time. Not many people will contribute to these files, and on the other hand, external services, if they have problems with stability, will not affect our CI. If we had tests that are run much less frequently, e.g. once a week, and not with any changes in the project, I would be glad to add these tests. |
Agree. Hopefully soon! 🤞 |
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.
LGTM
The PR is likely ready to be merged. No tests are needed as no important environment files, nor python files were modified by it. However, committers might decide that full test matrix is needed and add the 'full tests needed' label. Then you should rebase it to the latest master or amend the last commit of the PR, and push it with --force-with-lease. |
Do you think would be a good idea to have also docker-compose-recipes doc? |
@mik-laj works on a whole eeasy-to-generate docker-compose example to become part of the repo #8605 (comment) All comments are welcome :) |
The Workflow run is cancelling this PR. Building images for the PR has failed. Follow the the workflow link to check the reason. |
1030c07
to
4c46718
Compare
4c46718
to
9d52e45
Compare
(cherry picked from commit bfb7cb3)
hello,
Some developers ask how to install additional tools in the image. I have prepared instructions for a few common tools. These tools are required by some operators, but due to image size optimization, they are not included in the default installation.
Best regards,
Kamil Breguła
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.