feat: add config name support

[gaurpulkit]

Important

Add support for named configuration in project-health command, allowing use of configurations from the Altimate API with precedence rules and updated documentation.

• Behavior:
  • Add --config-name option to project-health command in cli.py to use named configurations from the Altimate API.
  • If both --config-path and --config-name are provided, --config-path takes precedence.
  • Handle missing access attribute in _get_node() in wrapper.py for v10, v11, and v12.
• API Client:
  • Add get_all_dbt_configs() to client.py to fetch DBT configurations from the API.
  • Add get_all_dbt_configs() utility function in utils.py.
• Documentation:
  • Update features.rst to include instructions for using --config-name and related options.

^{This description was created by for b691261. It will automatically update as commits are pushed.}

[ellipsis-dev]

👍 Looks good to me! Reviewed everything up to d282c53 in 3 minutes and 54 seconds

More details

• Looked at 95 lines of code in 3 files
• Skipped 0 files when reviewing.
• Skipped posting 10 drafted comments based on config settings.

1. src/datapilot/clients/altimate/client.py:108

• Draft comment:
  Consider adding error handling (try/except) for get_all_dbt_configs similar to other GET methods.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  The get_all_dbt_configs() method calls self.get() which already has extensive error handling. The comment appears to have missed this fact. Since the error handling is already implemented in the parent method, adding additional error handling would be redundant and potentially harmful as it would mask the existing error handling.
  I could be wrong if there are specific DBT-related errors that need special handling beyond what the base get() method provides. Also, the get() method doesn't re-raise exceptions which could be a valid concern.
  While those are valid points, the existing error handling in get() is comprehensive and follows a consistent pattern across the codebase. If special error handling was needed, it should be added to the base get() method rather than individual callers.
  The comment should be deleted as it suggests adding error handling that already exists through the parent get() method.

2. src/datapilot/clients/altimate/utils.py:129

• Draft comment:
  Consider adding type hints for parameters and return type for consistency.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50%
  None

3. src/datapilot/core/platforms/dbt/cli/cli.py:78

• Draft comment:
  Ensure token and instance_name are provided when --config-name is used; current condition silently returns if missing.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

4. src/datapilot/core/platforms/dbt/cli/cli.py:83

• Draft comment:
  Consider handling ambiguous results if multiple configs match --config-name.
• 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%
  The code could potentially have an issue if multiple configs share the same name, as it silently takes the first match. However, this seems like an edge case - config names are likely meant to be unique in the API. Without knowing the API contract or data model, we can't be certain this is actually a problem. The comment is speculative without strong evidence that duplicate names are possible or problematic.
  I may be underestimating the importance of handling edge cases. If config names aren't guaranteed unique in the API, silently taking the first match could lead to confusing behavior.
  While edge case handling is important, without evidence that duplicate config names are possible or that this is a real issue users face, this comment is too speculative. We should follow the rule of not making "if X, then Y" style comments without clear evidence.
  Delete this comment as it's speculative without strong evidence that duplicate config names are possible or that this is a real issue that needs addressing.

5. src/datapilot/clients/altimate/client.py:108

• Draft comment:
  The GET call in get_all_dbt_configs hardcodes the page size to 100. Consider making this configurable or implementing pagination if more than 100 configs are expected.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50%
  The method name get_all_dbt_configs combined with a fixed page size is potentially problematic. However, I don't have evidence that: 1) There will ever be more than 100 configs 2) The API doesn't handle pagination automatically 3) This is actually a bug rather than an intentional limit. Without this context, the comment is somewhat speculative.
  I might be too harsh - pagination is a common issue and the method name does promise "all" configs. The comment could prevent future issues.
  While pagination is important, we don't have enough context to know if this is actually a problem. The comment is speculative ("if more than 100 configs are expected") which violates our rules.
  The comment should be deleted as it's speculative and we lack context about whether this is actually a problem.

6. src/datapilot/core/platforms/dbt/cli/cli.py:78

• Draft comment:
  When 'config_name' is used, the code selects the first matching config without handling multiple matches. Clarify if only one is expected or consider handling duplicates explicitly.
• 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%
  The comment raises a valid point about duplicate handling, but it's somewhat speculative. We don't know if duplicate config names are even possible in the system - this might be enforced elsewhere. The code works fine as is, and if duplicate names are an issue, it would likely be caught during testing or actual usage. The comment is asking for clarification rather than pointing out a definite issue.
  I might be underestimating the importance of handling duplicate configs. If duplicate names are possible, silently taking the first match could lead to subtle bugs.
  While duplicate handling could be important, the comment is asking for clarification rather than pointing out a clear issue. Per the rules, we should not keep comments that ask authors to clarify their intentions.
  Delete the comment because it's asking for clarification and is speculative about whether duplicate configs are possible or problematic.

7. src/datapilot/core/platforms/dbt/cli/cli.py:88

• Draft comment:
  On API fetch failure or missing matching config, the CLI simply echoes an error and returns. For better CLI experience, consider using sys.exit with a non-zero status to signal an error.
• 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%
  This is a reasonable suggestion for CLI tools - using non-zero exit codes is a standard practice to signal errors to calling scripts/shells. The current implementation makes it harder to programmatically detect failures. However, looking at the rest of the file, other error cases like invalid credentials (L155) also just use echo+return, so this would be inconsistent with the established pattern in the codebase.
  The comment suggests changing established error handling patterns in the codebase. We don't know if there's a reason the codebase consistently uses echo+return instead of sys.exit().
  While using sys.exit() would be better practice, suggesting changes that would make error handling inconsistent within the same file is probably not helpful.
  The comment should be removed since it suggests deviating from the codebase's established error handling pattern without strong justification.

8. src/datapilot/clients/altimate/client.py:63

• Draft comment:
  Minor typographical issue: In the f-string on line 63, there's an extra space inside the curly brace ('{response.status_code }'). It should be '{response.status_code}'. This is trivial but cleaning it up would make the code look a bit neater.
• 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.

9. src/datapilot/clients/altimate/utils.py:87

• Draft comment:
  The error message on line 87 contains excessive trailing whitespace. Removing these extra spaces will make the message cleaner.
• 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.

10. src/datapilot/clients/altimate/utils.py:104

• Draft comment:
  The error message on line 104 contains excessive trailing whitespace. Removing these extra spaces will improve consistency.
• 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.

Workflow ID: wflow_ior3dTML9EyeXxFf

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 7eca8f5 in 1 minute and 20 seconds

More details

• Looked at 24 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 4 drafted comments based on config settings.

1. src/datapilot/clients/altimate/client.py:110

• Draft comment:
  Changing endpoint from '/dbt/v1/configs' to '/dbtconfig/' may affect backward compatibility. Confirm that the service expects this new endpoint and update documentation if needed.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  This comment violates several rules: 1) It asks for confirmation ("Confirm that...") which we explicitly shouldn't do 2) It's speculative about potential issues rather than pointing out a definite problem 3) It asks for documentation updates which isn't a clear code change 4) Without access to the API documentation or service, we can't be certain this is even an issue.
  The endpoint change could genuinely break things in production. Documentation updates are important for API changes.
  While those concerns are valid, our rules explicitly state not to make speculative comments or ask for confirmations. If this was a breaking change, it should be caught in testing or CI/CD.
  This comment should be deleted as it violates multiple comment rules by being speculative and asking for confirmation rather than pointing out a definite issue.

2. src/datapilot/core/platforms/dbt/cli/cli.py:86

• Draft comment:
  Added echo message for using the selected config. Confirm if user feedback via stdout is desired or if a debug log is preferable.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

3. src/datapilot/clients/altimate/client.py:110

• Draft comment:
  Ensure the updated endpoint '/dbtconfig/' is intentional and backward compatible with clients expecting '/dbt/v1/configs'.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the author to ensure that the change is intentional and backward compatible. It is not making a specific suggestion or pointing out a specific issue. It violates the rule against asking the author to ensure behavior is intended.

4. src/datapilot/core/platforms/dbt/cli/cli.py:86

• Draft comment:
  Using click.echo to notify the selected config is helpful, but ensure this output aligns with your CLI design (e.g., verbosity levels).
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

Workflow ID: wflow_iNqbFumXoAU359Yt

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 3933ac6 in 47 seconds

More details

• Looked at 32 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 7 drafted comments based on config settings.

1. docs/features.rst:46

• Draft comment:
  Good fix on the typo for "dbt commands".
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

2. docs/features.rst:49

• Draft comment:
  Consider clarifying 'instance-name' as 'tenant ID' if it differs from typical usage.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

3. docs/features.rst:66

• Draft comment:
  Confirm that providing both '--config-path' and '--config-name' as described is the intended behavior.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

4. docs/features.rst:45

• Draft comment:
  Improved phrasing consolidates directory and model selection clearly.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

5. docs/features.rst:47

• Draft comment:
  Typo fixed: changed 'comands' to 'commands'.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is purely informative as it only states that a typo was fixed. It does not provide any actionable feedback or suggestions for improvement.

6. docs/features.rst:49

• Draft comment:
  New 'Configuration' section with examples is a good addition—ensure list indentations and reST formatting are consistent.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

7. docs/features.rst:62

• Draft comment:
  Clarify terminology: '--instance-name' is described as 'Your tenant ID'. Ensure consistent naming to avoid confusion.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

Workflow ID: wflow_jNYOzKKmajthx5b3

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on b691261 in 44 seconds

More details

• Looked at 39 lines of code in 3 files
• Skipped 0 files when reviewing.
• Skipped posting 9 drafted comments based on config settings.

1. src/datapilot/core/platforms/dbt/wrappers/manifest/v10/wrapper.py:119

• Draft comment:
  Changed logic for accessing node.access.value looks correct; consider adding a brief inline comment to explain why getattr is used.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

2. src/datapilot/core/platforms/dbt/wrappers/manifest/v11/wrapper.py:119

• Draft comment:
  Consistent update using getattr for node.access; inline comment might benefit future readers.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

3. src/datapilot/core/platforms/dbt/wrappers/manifest/v12/wrapper.py:119

• Draft comment:
  The modification to safely obtain node.access.value using getattr is correct; consider a code comment to detail the necessity.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50%
  None

4. src/datapilot/core/platforms/dbt/wrappers/manifest/v10/wrapper.py:119

• Draft comment:
  Consider simplifying the attribute lookup. Instead of 'hasattr(node, "access") and node.access is not None ?', you could use: getattr(getattr(node, 'access', None), 'value', None).
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

5. src/datapilot/core/platforms/dbt/wrappers/manifest/v11/wrapper.py:119

• Draft comment:
  Consider using a simplified lookup: getattr(getattr(node, 'access', None), 'value', None) instead of the current conditional.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

6. src/datapilot/core/platforms/dbt/wrappers/manifest/v12/wrapper.py:119

• Draft comment:
  Consider refactoring the attribute access to: getattr(getattr(node, 'access', None), 'value', None) for brevity and consistency.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

7. src/datapilot/core/platforms/dbt/wrappers/manifest/v10/wrapper.py:405

• Draft comment:
  There are typographical errors in the docstring of the 'parent_to_child_map' function. For example, 'THis' should be 'This' and 'childre' should be 'children'. Please correct these errors.
• 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.

8. src/datapilot/core/platforms/dbt/wrappers/manifest/v11/wrapper.py:405

• Draft comment:
  Typographical error in the docstring of the parent_to_child_map function. Consider correcting 'THis gives an information of node to childre' to 'This provides information mapping nodes to children'.
• 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.

9. src/datapilot/core/platforms/dbt/wrappers/manifest/v12/wrapper.py:404

• Draft comment:
  There's a typographical error in the docstring of the parent_to_child_map method. The comment reads "THis gives an information of node to childre". Consider changing it to something like "This provides a mapping from each parent node to its children." This corrects 'THis' to 'This' and 'childre' to 'children'.
• 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.

Workflow ID: wflow_xrvd8focKfNfqSa8

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.