-
Notifications
You must be signed in to change notification settings - Fork 1
feat: fix project governance exposures issue and add integration name option enhancement #65
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
feat: fix project governance exposures issue and add integration name option enhancement #65
Conversation
…ate catalog loading logic
…imateExposureConfig
…ce onboarding options in CLI
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.
Important
Looks good to me! 👍
Reviewed everything up to bd9bca6 in 1 minute and 35 seconds. Click for details.
- Reviewed
299lines of code in13files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/datapilot/clients/altimate/client.py:110
- Draft comment:
Added get_all_integrations method; consider adding error handling similar to other GET methods. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. src/datapilot/clients/altimate/utils.py:149
- Draft comment:
New resolve_integration_name_to_id function; consider handling case-insensitive matches and adding logging when no integration is found. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% Case-insensitive matching could prevent user confusion if they get the casing wrong. Logging would help with debugging. However, these are more like nice-to-have improvements rather than critical issues. The current implementation is functionally correct - it will work as long as the exact name is provided. The rules say to not keep comments unless there is clearly a code change required. The function might be used in contexts where case-insensitive matching is actually important for usability. Silent failures could make debugging difficult in production. While these are valid concerns, they are more like optional improvements rather than clear bugs or issues that must be fixed. The function works correctly as implemented. The comment should be deleted as it suggests nice-to-have improvements rather than required changes. The current implementation is functionally correct.
3. src/datapilot/core/platforms/dbt/cli/cli.py:187
- Draft comment:
Integration name option enhancement is clear. If both integration ID and name are omitted, consider prompting for one; warning when both are provided is useful. - Reason this comment was not posted:
Comment looked like it was already resolved.
4. src/datapilot/core/platforms/dbt/wrappers/manifest/v12/wrapper.py:204
- Draft comment:
Changed exposure config to use AltimateExposureConfig instead of AltimateSourceConfig; ensure the new schema aligns with exposure data. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. src/datapilot/core/platforms/dbt/utils.py:91
- Draft comment:
Using CatalogV1(**catalog_dict) in load_catalog improves consistency; ensure that the JSON schema matches CatalogV1 expectations. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_cI83Jk4UlriM2Fx1
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
…w schema structure
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.
Important
Looks good to me! 👍
Reviewed 3e259e7 in 1 minute and 36 seconds. Click for details.
- Reviewed
138lines of code in7files - Skipped
0files when reviewing. - Skipped posting
7draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/datapilot/core/platforms/dbt/executor.py:18
- Draft comment:
Update Catalog import to use the new catalog module (schemas/catalog) instead of manifest. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, as it is simply stating what was done without providing any suggestions or questions for the author. It does not ask for any specific action or confirmation from the PR author, nor does it provide any constructive feedback or code suggestions.
2. src/datapilot/core/platforms/dbt/factory.py:7
- Draft comment:
Refactor Catalog and CatalogV1 imports to use the new catalog module for consistency. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. src/datapilot/core/platforms/dbt/schemas/catalog.py:60
- Draft comment:
Custom Metadata and CatalogV1 classes added with 'extra=allow'; consider adding inline documentation on why extra fields are allowed. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
4. src/datapilot/core/platforms/dbt/schemas/manifest.py:42
- Draft comment:
Removed custom Catalog definitions from manifest; ensure all downstream consumers now reference schemas/catalog. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. src/datapilot/core/platforms/dbt/utils.py:20
- Draft comment:
Update Catalog and CatalogV1 imports in utils to use the new module path. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, as it is simply stating what was done without providing any suggestions or questions for the author. It does not ask for any specific action or provide any insight into potential issues or improvements.
6. src/datapilot/core/platforms/dbt/wrappers/catalog/v1/wrapper.py:1
- Draft comment:
Update CatalogV1 import to use the new catalog module instead of manifest. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, suggesting an update to an import statement without providing a specific code suggestion or asking for confirmation of intent. It doesn't align with the rules for useful comments.
7. src/datapilot/utils/utils.py:15
- Draft comment:
Update CatalogV1 import to reflect its new location in schemas/catalog. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, as it is simply stating what was done without providing any suggestion, question, or request for confirmation. It does not align with the rules for useful comments.
Workflow ID: wflow_cfz8MbPJgLTZOa1v
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Important
Adds integration name resolution, updates project governance checks, and refactors catalog handling, with version bump to 0.0.23.
get_all_integrations()toclient.pyandutils.pyto fetch all integrations.resolve_integration_name_to_id()inutils.pyto map integration names to IDs.onboard()incli.pyto accept integration names as an alternative to IDs.project_health()incli.pyto fetch DBT configs by name.Catalogdefinition frommanifest.pytocatalog.py.MetadataandCatalogV1classes incatalog.pyto handle extra fields.parse_catalog()withCatalogV1instantiation inutils.pyandutils/utils.py.CatalogV1import fromdbt_artifacts_parserinfactory.py..bumpversion.cfg,conf.py,setup.py, and__init__.py.This description was created by
for 3e259e7. You can customize this summary. It will automatically update as commits are pushed.