Revamp QA checks into a battery included package

[alafanechere]

What

Relates to:

• #2849
• #2905
• #2908
• #2907

This PR introduces a new connectors-qa 🐍 package which can run static-analysis checks on our connectors and generate documentation.

It will help address the following problems we have:

• Our connectors standards are poorly documented
• Our QA checks (enforcing connector standards) are scattered in multiple packages:
  • The qa_checks.py script in the connector_ops package (called by airbyte-ci)
  • In airbyte-ci (e.g. VersionFollowsSemverCheck or CheckPythonRegistryPublishConfiguration)

🤔 Philosophy

This efforts is driven by the following principles:

• Our connector standards apply to all connectors:
  • Airbyte should commit that certified connectors will always meet all the standard.
  • Any new contribution on a community connector must meet all standards to be merged.
• We should limit as much as possible any connector specific logic inairbyte-ci. We should consider it an orchestrator calling external tools and packages. airbyte-ci will call a containerizedconnectors-qa via its CLI.

🎉 Net new features

Documentation generation

connectors-qa generate-documentation connectors_qa_documentation.md

This will generate a markdown filedocumenting all the enabled checks.
The content is taken from check classes names and descriptions.

Report generation

connectors-qa run --connector-directory=airbyte-integrations/connectors --report-path=qa_report.json

This will generate a json report of all the QA checks on all our connectors.
We could automate the generation of this report in our CD pipeline and use it to feed dashboards or other assets like connector's README.
And create cool badges like:

Recommended reading order

1. README.md to install and try out the tool locally.
2. checks/*.py to understand which checks are running
3. cli.py to grasp how the entrypoint and asyncio logic is implemented

🚨 User Impact 🚨

None as this PR is just introducing a new package which is not yet used by airbyte-ci

Follow up steps

0. Commit the generated documentation doc: Document our connectors QA checks #35324
1. Call this package in the QAChecks step of airbyte-ci connectors test
2. Remove connector_ops/qa_checks and airbyte-ci steps that are now running inside this package (semver version check, pypi publishing etc.)
3. Automate the report generation and host it on GCS to power dashboards - connector readme badges?
4. Replace the "Connector checklist" on PR by a comment linking to the generate doc?

[vercel]

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

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	⬜️ Ignored (Inspect)	Visit Preview		Feb 19, 2024 9:28am

[alafanechere]

• master
  • Revamp QA checks into a battery included package #35322 👈
    • airbyte-ci: use connectors-qa instead of connector_ops.qa_check #35325
    • doc: Document our connectors QA checks #35324

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @alafanechere and the rest of your teammates on Graphite

[bazarnov]

Looks very promising to me, thanks @alafanechere ! Not approving now, because i'll get a closer look tomorrow to see the big picture of the checks themselves.

[bazarnov]

What holds our back to switch to the pydantic 2.0.0? Just curious?

[alafanechere]

I don't know, nothing I'm aware of, but it's a different package / project than the current one :)

[natikgadzhi]

I don't yet know the full surface area of us using Pydantic. If we're using 1.* everywhere, should we treat 2.0+ upgrade as a separate lane of work, or do you mostly expect things to work smoothly with such an upgrade?

Is is drop-in compatible syntax-wise?

If we're using it in the CDK itself and in airbyte-ci — I think airbyte-ci could be our guinea pig for such a migration? /cc @bazarnov

[bazarnov]

I think the only package that still demands the 1.* version is CAT, and yet, let the airbyte-ci be the guide indeed.

[bazarnov]

Do we have all the critical rules covered?

[alafanechere]

I ported over what currently exists in qa_checks.py . Let me know if you think additional checks should be implemented. I'm open for it, but in different PRs and with related new GH issues 😄

[natikgadzhi]

This does look great to me — since the potential blast radius of just having the tool is close to zero, I'm comfy approving this.

Caveats: I want follow-up work of:

• Publishing the actual generated documentation
• Moving our doc guide to our doc pages instead of hackmd
• Actually switching to use connectors-qa.

Please wait for @bazarnov's review and work with him to get a go ahead from API Sources, but I'm personally happy.

[natikgadzhi]

Why downgrade?

[alafanechere]

connectors_qa depends on two other internal packages which are using pydash but on different version:

• medata_service/lib uses v6
• connector_ops uses v7

I'm aligning to metadata_service/lib version because this package is also a dependency of metadata_service/orchestrator, which is also declaring a dependency on pydash.

Aligning to v6 makes me modifying a single package (connector_ops) instead of 2 (metadata_service/lib / metadata_service/orchestrator). A part from that I don't think there's a good reason to stay on v6.

[natikgadzhi]

Two questions here:

1. Does connectors_qa have to live in this particular directory, or is it just logical spot for it in the monorepo? I.e. could we just put it in the root of the repository instead?
2. If it's fully independent of the rest of connector_ops stuff (don't think so), is there a good overall readme and list of directories and what project is where? We have many things — registry service, airbyte-ci, etc.

[alafanechere]

@natikgadzhi I put it there because it's a package used in the CI context. But we could put it at the root of the repo, I don't mind 😄 .
It's dependent on a couple of other internal packages. I will explicitly list the dependency and why they exists in this README.

[natikgadzhi]

I was about to suggest that you could have a list of lists and then flatten, but then I remembered how flattening lists in Python is expressed kek.

[natikgadzhi]

How is Gradle used internally, and for what?

[alafanechere]

@natikgadzhi these checks run also on our java connectors which are using gradle.
This file is used in this program to determine if a connector is a java one.

[natikgadzhi]

Wow wow wow hold up, why are our standards in a 3rd party service and not a page on our very own docs site? Any specific reason? /cc @girarda @alafanechere

[girarda]

No idea. This is the first time I see this page. I think this was hacked together by dev-rel two years ago, but that's only based off this slack thread.

[alafanechere]

@natikgadzhi @girarda this doc was written by our previous documentation team. I believe it was drafted when I was working on the original qa_checks.py and never got checked in our main repo. I believe the doc is good enough to join our Resources section of Contributing to Airbyte. I can do it in a follow up PR.

[alafanechere]

This does look great to me — since the potential blast radius of just having the tool is close to zero, I'm comfy approving this.

Caveats: I want follow-up work of:

• Publishing the actual generated documentation
• Moving our doc guide to our doc pages instead of hackmd
• Actually switching to use connectors-qa.

Please wait for @bazarnov's review and work with him to get a go ahead from API Sources, but I'm personally happy.

@natikgadzhi I generated the documentation + add an integration test to make sure it's kept up to date in #35324

[bazarnov]

LGTM. Thanks @alafanechere for this improvement.