feat(ingest): add python deps for apk
#11188
Conversation
Will help us support Alpine images in the future.
github-actions
bot
added
ingestion
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes involve modifications across several scripts and a Dockerfile, focusing primarily on the installation commands for Python packages and system dependencies. The updates include the use of quotes for version specifications and the addition of Alpine Linux compatibility in a dependency installation script, enhancing flexibility without altering the overall functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Dockerfile
participant InstallScript
participant SmokeTest
User->>Dockerfile: Initiate build
Dockerfile->>InstallScript: Install dependencies
InstallScript->>Dockerfile: Return installed dependencies
Dockerfile->>SmokeTest: Setup environment
SmokeTest->>Dockerfile: Return environment setup confirmation
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| @@ -19,7 +19,13 @@ else | |||
| xz-devel \ | |||
| libxml2-devel \ | |||
| libxslt-devel \ | |||
| krb5-devel | |||
| krb5-devel | |||
| elif command -v apk; then | |||
There was a problem hiding this comment.
This works but I think it would be better to check for distribution vs apk/yum command as there are other distros which has yum but the package names are different. -> https://www.cyberciti.biz/faq/find-linux-distribution-name-version-number/
This is more an fyi
There was a problem hiding this comment.
I think for apk it's fine since it's the alpine package keeper. Didn't know that yum could have issues - it hasn't been an issue thus far so I think I'll leave it as-is, but good to be aware of that
Actions performedReview triggered.
|
|
@coderabbitai review |
Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- docker/datahub-ingestion-base/Dockerfile (1 hunks)
- metadata-ingestion/scripts/install_deps.sh (1 hunks)
- smoke-test/smoke.sh (1 hunks)
Files skipped from review due to trivial changes (2)
- docker/datahub-ingestion-base/Dockerfile
- smoke-test/smoke.sh
Additional comments not posted (1)
metadata-ingestion/scripts/install_deps.sh (1)
23-28: Ensure compatibility with other package managers.The addition of the
apkblock enhances support for Alpine Linux. However, consider the existing comment about checking for distribution names instead of commands to improve compatibility with other distros that might useyumwith different package names.Ensure that the package names are correct for Alpine Linux and consider potential differences in other distributions.
hsheth2
added
the
merge-pending-ci
Will help us support Alpine images in the future.
Also fixes the quotes around the
uvinstall to avoid creating dummy files.Checklist
Summary by CodeRabbit
New Features
Bug Fixes
uvpackage version in installation commands to ensure proper interpretation by the shell.