Skip to content

Conversation

@AdiGajbhiye
Copy link
Contributor

@AdiGajbhiye AdiGajbhiye commented Sep 30, 2024

Important

Adds permission check to onboarding in cli.py and changes HTTP error logging level in client.py.

  • Behavior:
    • Adds validate_permissions() in client.py and utils.py to check user permissions.
    • Updates onboard() in cli.py to include permission check using validate_permissions().
    • Logs error message if permissions are insufficient.
  • Logging:
    • Changes logging level from error to debug for HTTP errors in get() in client.py.

This description was created by Ellipsis for 25649dc. It will automatically update as commits are pushed.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Reviewed everything up to 5f3c9cd in 21 seconds

More details
  • Looked at 68 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. src/datapilot/core/platforms/dbt/cli/cli.py:111
  • Draft comment:
    The validate_credentials function returns a Response object, not a boolean. You should check the response status code or content to determine if the credentials are valid.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_qqCzNZxavW7huIRF


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

click.echo("Error: Invalid credentials.")
return

if not validate_permissions(token, backend_url, instance_name):
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The validate_permissions function returns a Response object, not a boolean. You should check the response status code or content to determine if the permissions are valid.

endpoint = "/dbt/v3/validate-credentials"
return self.get(endpoint)

def validate_permissions(self):
Copy link
Collaborator

@suryaiyer95 suryaiyer95 Sep 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validate_upload_to_integration

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on 25649dc in 16 seconds

More details
  • Looked at 27 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. src/datapilot/clients/altimate/utils.py:55
  • Draft comment:
    The function validate_permissions should call validate_upload_to_integration instead of validate_permissions.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    The comment accurately describes the change made in the diff, as the function now calls validate_upload_to_integration. This suggests that the comment is correct and relevant to the change. There is strong evidence that the comment is about a change made in the diff and is not speculative or asking for confirmation.
    The comment might be seen as stating the obvious since the change is already made in the diff. However, it does confirm that the change aligns with the intended function behavior.
    Even if the comment seems obvious, it confirms the correctness of the change, which is valuable in a code review context.
    The comment is correct and relevant to the change made in the diff. It should be kept as it confirms the intended behavior of the function.

Workflow ID: wflow_fmZ8gcDFJ06iL4ox


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

@AdiGajbhiye AdiGajbhiye merged commit 250290a into main Oct 2, 2024
@AdiGajbhiye AdiGajbhiye deleted the fix-add-onboard-permission branch October 2, 2024 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants