-
Notifications
You must be signed in to change notification settings - Fork 4.8k
fix: allow MDX imports and frontmatter before migration guide title #67594
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
base: master
Are you sure you want to change the base?
fix: allow MDX imports and frontmatter before migration guide title #67594
Conversation
This change makes the migration guide validation more flexible by: - Adding a get_first_heading() helper that skips MDX imports, YAML frontmatter, and empty lines - Updating CheckMigrationGuide to use this helper instead of checking only the first line - Preserving the requirement for a proper Migration Guide title This fixes the validation failure for source-zendesk-support which has an MDX import on line 1. Requested by: Ian Alton ([email protected]) Related PR: #67077 Co-Authored-By: [email protected] <[email protected]>
Original prompt from [email protected]
|
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. Helpful Resources
PR Slash CommandsAirbyte Maintainers (that's you!) can execute the following slash commands on your PR:
|
/format-fix
|
/run-connector-tests
|
- Split test_fail_when_migration_guide_file_does_not_start_with_correct_header into two tests: - test_fail_when_migration_guide_file_is_empty: Tests empty file case - test_fail_when_migration_guide_file_does_not_start_with_correct_header: Tests wrong header case - Add test_pass_when_migration_guide_has_mdx_imports_before_title: Verifies MDX imports are properly skipped before title validation All tests now align with the updated get_first_heading() implementation that skips frontmatter, MDX imports, and empty lines before checking for the migration guide title. Co-Authored-By: [email protected] <[email protected]>
Co-Authored-By: [email protected] <[email protected]>
(aside) The current connector test for migration guides is extremely strict. It does not permit the use of MDX imports or front matter. However, these are pretty standard in Docusaurus and it's better that we expect them. Rather than trying to move the import around to satisfy the test, I've had Devin update the test and add a new helper function. It still requires the article start with a proper heading as before, but it's no longer necessary for that heading to be on line 1 if front matter is present in the topic. I simulated a connector update with #67595 and all checks passed. |
migration_guide_file_path=tmp_path / "migration_guide.md", | ||
) | ||
connector.migration_guide_file_path.write_text( | ||
"import SpecialDoc from '@site/src/components/SpecialDoc';\n\n" "# Test Connector Migration Guide\n" "## Upgrading to 1.0.0\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For our purposes here, does this need to be an actual functioning import with a real file? Or is a mock enough? IE I guess what I'm saying is if this were a real Docusaurus instance, this wouldn't build. But it's OK if we don't expect it to.
Hi Ian! Great question. A mock import is sufficient for this unit test. The test is specifically validating the text parsing logic in
The validation doesn't attempt to resolve or validate the actual MDX imports - it just needs to recognize and skip the line pattern. The mock import For real-world verification, I created PR #67595 which recreates changes to source-zendesk-support (which has a real MDX import in its migration guide). All CI checks passed, confirming the validation works correctly with actual MDX imports in production. So to answer your question: the mock is fine for this unit test context, and we don't need the import to be buildable by Docusaurus. ✅ |
What
Fixes migration guide validation to handle MDX imports and YAML frontmatter that precede the main title. Currently, the validation expects the migration guide title to be on line 1, but valid guides may have metadata or imports before the title.
This addresses the failing test in PR #67077 where
source-zendesk-support-migrations.md
begins with an MDX import statement on line 1, causing validation to fail despite having a proper title on line 3.How
Added
get_first_heading()
helper function inhelpers.py
:---
markers)import
)#
Updated
CheckMigrationGuide._run()
indocumentation.py
:Review guide
airbyte-ci/connectors/connectors_qa/src/connectors_qa/checks/documentation/helpers.py
airbyte-ci/connectors/connectors_qa/src/connectors_qa/checks/documentation/documentation.py
Testing considerations:
source-zendesk-support
caseUser Impact
Positive: Migration guides with valid MDX imports or YAML frontmatter will now pass validation correctly, unblocking connector releases.
Potential risks: The parsing logic could have edge cases that incorrectly skip over actual titles, though this is unlikely given the simple line-by-line approach.
Can this PR be safely reverted and rolled back?
This change only makes validation more permissive for valid content patterns. Reverting would restore the original strict first-line validation without breaking existing functionality.
Link to Devin session: https://app.devin.ai/sessions/d66e3af38e1d4d5396c475c9215e827f
Requested by: Ian Alton ([email protected])