-
Notifications
You must be signed in to change notification settings - Fork 415
Skip doc examples requiring secrets on fork PRs #3438
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
Conversation
Fixes CI failures for external contributors
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
docs | 8b884fa | Commit Preview URL Branch Preview URL |
Dec 05 2025, 02:51 PM |
| args = parser.parse_args() | ||
|
|
||
| # Check if CI is running on a fork pull request | ||
| is_fork = os.environ.get("IS_FORK") == "true" |
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.
we have a helper:
Line 551 in d9e15a7
| def is_running_in_github_fork() -> bool: |
def is_running_in_github_fork() -> bool:
"""Check if executed by GitHub Actions, in a repo fork."""
is_github_actions = is_running_in_github_ci()
is_fork = os.environ.get("IS_FORK") == "true" # custom var set by us in the workflow's YAML
return is_github_actions and is_fork
```
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.
Good point! However, the prepare_examples_tests.py script is in docs/docs_tools/examples/ and isn't a pytest test file, rather a standalone script that generates test files. Importing from tests/utils would create an awkward dependency from the docs tooling on the test utilities module. Additionally, the existing helper is_running_in_github_fork() does an extra check for GITHUB_ACTIONS == "true" via is_running_in_github_ci, but here we only need to check IS_FORK since this script only runs in CI anyway (and having IS_FORK=true already implies ci context)
If we want to keep things consistent, we could either:
- Keep it as-is (simple inline check, no dependency)
- Extract a shared utility for fork detection that both modules can use
I'd lean toward keeping it simple here, but happy to refactor if you prefer.
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.
ah true... lets definitely not import that. option 2 is fine!
| "chess_production", | ||
| "custom_destination_bigquery", | ||
| "custom_destination_lancedb", | ||
| "custom_naming", |
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.
learning question: why this example?
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.
custom_naming example uses dlt.destinations.postgres() which requires Postgres credentials
| dest_ = dlt.destinations.postgres(naming_convention="sql_cs_latin2") |
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.
makes sense...so its just a bit inconsistent that this example doesnt include a example.secrets.toml
| "custom_naming", | ||
| "google_sheets", | ||
| "incremental_loading", | ||
| "nested_data", |
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.
question:partial_loading also uses aws creds so should be in there?
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.
partial_loading example, it uses local filesystem storage, not aws/s3 creds:
| dest_ = dlt.destinations.filesystem("_storage") |
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.
ah right, and in filesystem the bucket_url (assigned above) is used to determine the kind of credential needed (and local filesystem doesnt require any so nothing ever gets resolved)
djudjuu
left a comment
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.
its an improvement. i guess we always pay special attention when merging outside stuff.
I assume tests also run on the merged devel branch so we should see failing examples there as well...or on the next pr that comes after and doesnt touch the code (not ideal, but better than now)
also: left two questions and a nitpick
|
Thanks for the review @djudjuu, afaik the tests won't rerun on merge to devel. |
|
When a PR comes from a fork, GitHub Actions doesn't expose repository secrets. This causes the docs examples tests to fail because many examples require external credentials (Postgres, BigQuery, Slack webhooks, etc.).
This PR adds an
EXAMPLES_REQUIRING_SECRETSlist toprepare_examples_tests.pythat skips these examples whenIS_FORK=true.