Skip to content
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

Add error surfacing for git cloning errors #4124

Merged
merged 4 commits into from
Nov 5, 2021

Conversation

Gitznik
Copy link
Contributor

@Gitznik Gitznik commented Oct 24, 2021

resolves #3167

Description

  1. Use the existing dbt.exceptions.bad_package_spec() function to raise the git error and print the content to stdout.
    Decided to do this in the clone method as that is where it is done for the checkout part.
    If git returns the usage: git string with the error, I remove it from the message as it is very verbose and probably not helpful.
  2. On an unknown error during cloning, the user is now advised to take a look at the debug logs for more information on the error

This handled the errors I managed to test fine:

  • Invalid Repo Name
  • Too many arguments as described by @tpilewicz
  • Invalid revision (still works as it did before)

I did not test:

  • A bad installation of git. Let me know if you know of a way to test this.

I think this works well enough for surfacing the error messages git provides, without any more contextual error handling. OSErrors should be raised as they were before.

If you think I should add unit tests for this, let me know which file/test class would be appropriate to add them. I could only find test_deps.py which did not seem to test any git functionality.

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have updated the CHANGELOG.md and added information about my change

@cla-bot cla-bot bot added the cla:yes label Oct 24, 2021
Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

@Gitznik Thanks for the contribution! This is a great step in the direction of more helpful error messages.

I'm going to move up the changelog entry now. Otherwise, this lgtm!

Edit: this is tricky without first rebasing/pulling changes from main. I'll plan to reorganize the changelog ahead of cutting rc1 next week.

@jtcohen6 jtcohen6 merged commit c7bc6eb into dbt-labs:main Nov 5, 2021
@Gitznik
Copy link
Contributor Author

Gitznik commented Nov 5, 2021

Awesome, thank you :) Looking forward to contributing more.

iknox-fa pushed a commit that referenced this pull request Feb 8, 2022
* Add error surfacing for git cloning errors

* Update CHANGELOG.md

* Fix formatting and remove redundant except: raise

* Turn error handling for duplicate packages back on

automatic commit by git-black, original commits:
  c7bc6eb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

If dbt deps can't git clone, it doesn't give the error message encountered by git
2 participants