Skip to content
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

fix: use root for python installs #49136

Merged
merged 14 commits into from
Dec 12, 2024

Conversation

aaronsteers
Copy link
Collaborator

@aaronsteers aaronsteers commented Dec 11, 2024

Attempts to resolve: https://airbytehq.sentry.io/issues/6129237224/?project=4505596642983936

What

How

Review guide

User Impact

Can this PR be safely reverted and rolled back?

  • YES 💚
  • NO ❌

Copy link

vercel bot commented Dec 11, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
airbyte-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 12, 2024 5:02pm

@aaronsteers aaronsteers marked this pull request as ready for review December 11, 2024 20:29
@aaronsteers aaronsteers requested a review from a team as a code owner December 11, 2024 20:29
@aaronsteers aaronsteers requested review from erohmensing and alafanechere and removed request for a team December 11, 2024 20:29
@natikgadzhi
Copy link
Contributor

Hold up. I understand this is the consequence of having non-root connectors, with @alafanechere?

I would love to make the path writable for user airbyte in the base image instead of falling back to using root.

@aaronsteers
Copy link
Collaborator Author

@natikgadzhi - My understanding is that we don't want the user account used at runtime to have permissions to modify anything in the container - and I think that includes installing packages and modifying source code. For this reason, I think it is correct for root to be required for installations, so a toggle to root and back seems correct when we need to modify the image by installing packages or injecting other code that should not be able to be modified at runtime. Definitely happy to continue this conversation in other threads/channels...

@alafanechere - I got pulled into other projects this afternoon but I'm wrapping up this evening with an added version bump and changelog so this is ready for you to merge tomorrow when you start your business hours. As noted in DM, I wasn't sure how to test prior to merging. I believe you can simply merge this and then retest during your business hours - creating a new PR for follow-up if necessary. (Biasing for faster time-to-resolution instead of fewer merges, since the tests are already broken.)

@alafanechere alafanechere force-pushed the aj/airbyte-ci/fix/use-root-for-python-installs branch from e3bed05 to 515214c Compare December 12, 2024 10:56
@alafanechere
Copy link
Contributor

alafanechere commented Dec 12, 2024

@aaronsteers So after some research my conclusion is:

  • At test time we install dev dependencies on top of a python container built from our non root base image. The current user is airbyte.
  • As we disable venv creation in our docker image poetry tries to install the dev dependencies at the system level. But as the current user is airbyte it has no permission to write to system packages.
  • If we used pure pip to install dependency the workaround would be to use pip install --user and/or set PYTHONUSERBASE=/airbyte. This was my original attempt in this commit 515214c
  • Poetry does not have a way to install packages in the user space when the venv creation is disabled. Install globally with --user python-poetry/poetry#1214
  • Given that these operation on the container are performed a "test environment provisioning" time it is fine switching the use to root to run the dev deps install and switch back to root afterward.
  • I'll revert my commit 515214c

@alafanechere alafanechere force-pushed the aj/airbyte-ci/fix/use-root-for-python-installs branch from 515214c to 68978e6 Compare December 12, 2024 12:36
@alafanechere
Copy link
Contributor

alafanechere commented Dec 12, 2024

Running tests on source-shopify and source-pokeapi to confirm the issue is resolved https://github.com/airbytehq/airbyte/actions/runs/12300651088

@aaronsteers
Copy link
Collaborator Author

Thanks for keeping this one moving forward, @alafanechere. I can't officially approve this PR because I was the original author - but please feel free to merge when ready. ✅ 👍

@alafanechere
Copy link
Contributor

Shopify unit tests passed on this run (which correctly used airbyte-ci installed from sources)

@alafanechere alafanechere merged commit d75e6ed into master Dec 12, 2024
30 of 32 checks passed
@alafanechere alafanechere deleted the aj/airbyte-ci/fix/use-root-for-python-installs branch December 12, 2024 17:23
barduinor pushed a commit to box-community/airbyte that referenced this pull request Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants