feat(qa_check): enable checking connector docs structure via qa check

[darynaishchenko]

What

as part of https://github.com/airbytehq/airbyte-internal-issues/issues/2887

How

Added tests for validation critical connectors docs structure to existing CheckDocumentationStructure test suite.
These test were previously implemented in cat #34380. But cat's structure doesn't allow to pass connector docs file into cat container, so tests were moved to qa checks.

Review guide

User Impact

Can this PR be safely reverted and rolled back?

• YES 💚
• NO ❌

[vercel]

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	Aug 14, 2024 4:04pm

[alafanechere]

I really appreciate the effort to make our docs more consistent.
But I have some blocking comments:

• We can't reliably get a connector spec before the connector build. The spec source of truth is the spec command output, not its json/yaml file. I unfortunately think there's no easy way to implement such a check on the pre-requisite section.
• You hardcoded a "skip the check for some connectors" logic, but you could re-use some existing class attribute we use as flags to only apply the check on a subset of connectors.
• I think the CheckDocumentationStructure has grown too big. It'd benefit maintainability and documentation if you'd declare more granular and new checks.
• Can you make sure to follow README instruction to introduce a new check. I think you're missing a version bump and the regeneration of the QA checks docs.
• I'd also recommend manually running the connectors-qa run --connector-directory=airbyte-integrations/connectors command to make sure the checks are passing for all connectors. To not break connectors' nightly build and parallel efforts I think the failing connectors should be fixed before introducing the check.

[darynaishchenko]

@alafanechere, Thanks for the review!

I'd also recommend manually running the connectors-qa run --connector-directory=airbyte-integrations/connectors command to make sure the checks are passing for all connectors. To not break connectors' nightly build and parallel efforts I think the failing connectors should be fixed before introducing the check.

Thanks, it was one of my future questions. I'll check it .
I also prepared a pr for fixing connector docs #39328 I want to merge docs fix before the update for qa checks.

As I mentioned in an earlier comment, I don't think we have reliable way to consume the connector spec before the connector build or release.

This test suite is now applicable for certified connectors (required fields description is one of certification criteria). I guess we can just skip testing of Prerequisites description if connector doesn't have spec file.

In other words: the spec source of truth should be the spec command output, not anything declared in spec.yaml / spec.json / manifest

I totally agree with that. But it is a test that is requires both connector image and connector docs file. In cat we can't use docs file and in qa check we can't use spec command(please correct me if we can). For now it's only the facebook-marketing source which requires to run spec command to test Prerequisites description,so this test is being skipped for the source.

[alafanechere]

I guess we can just skip testing of Prerequisites description if connector doesn't have spec file.

@darynaishchenko That's sounds like a fair workaround to me if you think the file content is reliable enough.
Please add a comment in your code to share this "source of truth" issue.
If there are too much false positive/negative we can disable this check later.

[darynaishchenko]

@alafanechere I have run qa check on "critical" connectors on my branch with a fix for docs. All docs passed new qa check except google-sheets(1 invalid links was added, and I will fix it in my branch).
cmd: airbyte-ci-dev --disable-update-check connectors --name=source-amazon-ads --name=source-facebook-marketing --name=source-google-ads --name=source-google-analytics-data-api --name=source-google-search-console --name=source-google-sheets --name=source-hubspot --name=source-intercom --name=source-linkedin-ads --name=source-salesforce --name=source-shopify --name=source-stripe --name=source-tiktok-marketing --name=source-zendesk-support test --only-step="qa_checks"
Also ran connectors-qa run --connector-directory=airbyte-integrations/connectors, checks were running as expected but stuck on source-freshservice. It's passed when I run check only for source-freshservice. Do we have an option to skip some connectors when running connectors-qa run ?

[alafanechere]

@darynaishchenko can you please fix the units test and type check before I give a final review 🙏

[darynaishchenko]

@alafanechere, fixed unit tests and type checks

[alafanechere]

Thanks @darynaishchenko for the changes, it's going in the right direction. Unfortunately I still believe these checks lack explicitness, which is key in the role of this package. I'd love checks to be more granular and better documented so that as a connector developer, if I face a failure, I can easily relate to the qa-checks.md doc to figure how to fix it. And also, as a the maintainer of this package I'd clearly understand the scope and role of this check.

My recommendation would be to ideally:

• Create one check per documentation section you validate
• Use a single template file which stores the expected documentation strucutre, so that we can use it as the source of truth for a valid source structure. It could also be re-used in the connector generator.

I understand it's pretty hard to validate unstructured data like this doc.
The flow could be something like:

• A. Validate the overall doc structure (existence of each section)
• B. If the doc structure is valid we can transform serialize the doc in a more structured data format in memory (a dict with sections as key?)
• C. Feed additional per section checks with the "more structured" documentation.

I believe decoupling parsing and validation would help in boosting the readability of this code.

My other comments are rather not blocking cosmetics changes.

[darynaishchenko]

@alafanechere, I temporary changed url to standard template in qa-checks.md, Versel check failed because of invalid link: template.md is not found (I think). I'll update it to template.md once these changes are merged.