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

Upgrade python packages to use pytest 8 and requests 2.31 #36595

Closed
wants to merge 2 commits into from

Conversation

natikgadzhi
Copy link
Contributor

What

I'm working to resolve security alerts on the main Airbyte repo. This PR addresses ReDoS in py and vulnerabilities in requests.

Arguably, we should probably bundle cryptography in this as well — it has quite a few alerts with it.

How

  1. Unpins pytest dependency in setup.py files for connectors that have not moved to Poetry yet.
  2. Pins pytest pin with ^8 for all Poetry packages in our repository, along with pytest-mock to ^3.
  3. Runs tools/bin/run_poetry_lock.sh that poetry lock all packages.
  4. Resolve inconsistencies — connector_ops and qa_engine (soon to be removed in another PR) are frequent offenders

Risks

  • I'm not that scared about pytest in connectors. Worst case, their unit tests will start failing, and it should be verifiable by running pytest against them and checking previous version vs this version.
  • I'm more worried about airbyte-ci/pipelines — I've upgraded requests to 2.31 to resolve another requirement, but noticed that Ben pinned it to 2.28 because it was not compatible with previous version of docker package. We should test that airbyte-ci still works fine.

Should we republish all connectors for this?

No, we shouldn't — pytest is a dev dependency, and no behavior change is introduced. That's why I think that upgrading cryptography is a different concern — for that one we might actually want to republish.

@natikgadzhi natikgadzhi requested review from lazebnyi, oustynova and a team as code owners March 28, 2024 04:59
Copy link

vercel bot commented Mar 28, 2024

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

1 Ignored Deployment
Name Status Preview Updated (UTC)
airbyte-docs ⬜️ Ignored (Inspect) Mar 28, 2024 5:00am

@octavia-squidington-iii octavia-squidington-iii added area/connectors Connector related issues CDK Connector Development Kit labels Mar 28, 2024
@octavia-squidington-iv octavia-squidington-iv requested a review from a team March 28, 2024 05:01
Copy link
Contributor

@alafanechere alafanechere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI is mostly 🟢 on internal package.
I think you introduced a regression on CAT tests and pipelines type checks.
Screenshot 2024-03-28 at 10 45 32
On CAT the problem is most likely here

AttributeError: module 'py' has no attribute 'io'

py.io is likely something bundled with the old pytest version.
We must find a different way to get the terminal max width.

Small CI feedback:
I'd suggest creating multiple PRs for this kind of change.
One for connectors only on which you'd use [skip ci] commit prefix to avoid running costly CI on all our connectors.
A different one for internal packages where we actually care about CI.

@natikgadzhi
Copy link
Contributor Author

@alafanechere, thank you so much! First off, I agree, I borked a type check in pipelines. I'll look at what's borked in py.

And yeah, I'll redo this and cover just the connectors. Perhaps even separate out setup.py ones from Poetry ones.

@natikgadzhi
Copy link
Contributor Author

I'm not actively looking into this yet. I think perhaps a better way is to spend time on moving all connectors from Dockerfiles and setup.py to modern tooling first, and then move the pytest infra to modern version after that.

@natikgadzhi natikgadzhi closed this Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues CDK Connector Development Kit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants