-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Fix source seed selection docs generate #9454
Conversation
Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #9454 +/- ##
==========================================
- Coverage 87.96% 87.94% -0.02%
==========================================
Files 166 166
Lines 22094 22106 +12
==========================================
+ Hits 19435 19442 +7
- Misses 2659 2664 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
4861751
to
5634453
Compare
6d17b50
to
2b4b810
Compare
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.
LGTM, 1 nit.
) | ||
selected_nodes.extend(source_ids) | ||
selected_node_ids.update(selected_source_ids) | ||
selected_nodes.extend(selected_source_nodes) |
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.
Nit: Can we use tuple unpacking here?
selected_nodes = (*selected_nodes, *selected_source_nodes)
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.
this led to some unsavory mypy issues, so I left it as-is.
The backport to
To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-1.7.latest 1.7.latest
# Navigate to the new working tree
cd .worktrees/backport-1.7.latest
# Create a new branch
git switch --create backport-9454-to-1.7.latest
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 719a50cc91bc7a2a219b6fa4209e892f14177eb6
# Push it to GitHub
git push --set-upstream origin backport-9454-to-1.7.latest
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.7.latest Then, create a pull request where the |
(cherry picked from commit 719a50c)
resolves #9161
Problem
Sources are not actively participating in resource selection in docs generate. They cannot simply be included as nodes in the ResourceTypeSelector because then the task would attempt to compile a SourceDefinition which leads to all sorts of errors since they are not executable.
Seed selection actually does work, but in the edge case where a seed and source have the same database identifier, and the seed is selected, its catalog metadata is populated under
catalog.sources
instead of incatalog.nodes
. Vice versa happens if the source is selected but the seed is not. If both are selected, both are in the catalog as expected (already tested).Solution
Use the
ResourceTypeSelector
withSourceDefinition
during catalog generation, after compilation to include queried sources in the catalog result.Handle edge case by accepting the selected_ids to
catalog.make_unique_id_map
to only create node and source maps for selected seeds. Update the seed selection test to explicitly test selection for a seed that shares a relation with a source.Checklist