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

Fix dbt deps issues (#778 #994 #895) #1110

Merged
merged 10 commits into from
Nov 14, 2018
Merged

Conversation

beckjake
Copy link
Contributor

@beckjake beckjake commented Nov 6, 2018

Fix some issues around dbt deps.

I think this fixes #778, fixes #994, fixes #895 - some of those are sort of judgement calls

Behavior after this PR

  • When git is not available, dbt is more helpful now
> dbt deps
Encountered an error:
Could not find command, ensure it is in the user's PATH: "git"
  • When a repository is incorrect, shows the command that failed (it'd be nice to remove the username/password prompt!):
> dbt deps
Username for 'https://github.com': beckjake
Password for 'https://beckjake@github.com':
Encountered an error:
Got a non-zero returncode running: ['git', 'clone', '--depth', '1', 'https://github.com/fishtown-analytics/dbt-utilz.git', '23da7ac89d7d5c4086737ad8dc02feff']
  • When a revision is invalid, shows the ref that could not be found and the package URL:
> dbt deps
Encountered an error:
Error checking out spec='0.9.17' for repo https://github.com/fishtown-analytics/dbt-utils.git
fatal: Couldn't find remote ref 0.9.17
  • Uses shutil.move instead of os.rename to move things out of the download directory, to handle the cross-drive issue

  • By default, downloads go to a temp directory dbt-downloads-xxxxx where xxxxx is some random stuff, created by tempfile.mkdtemp that is removed via shutil.rmtree after a successful run. This should resolve the concurrent users issue. Users can override this behavior to download to a fixed directory using DBT_DOWNLOADS_DIR, in which case dbt will not shutil.rmtree the directory.

  • dbt no longer ignores return codes and instead raises exceptions that have stderr/stdout

  • More explicitly log what failed and where on permissions-related execution failures, this should be especially more helpful on windows

@beckjake beckjake force-pushed the fix/dbt-deps-issues branch 4 times, most recently from 109c24a to 24a4224 Compare November 6, 2018 22:52
@beckjake beckjake changed the base branch from dev/guion-bluford to dev/grace-kelly November 6, 2018 22:53
@beckjake beckjake force-pushed the fix/dbt-deps-issues branch from 4532425 to af52ebf Compare November 6, 2018 23:24
@beckjake beckjake changed the title Fix/dbt deps issues [WIP] Fix dbt deps issues (#778 #994 #895) Nov 6, 2018
@drewbanin drewbanin self-requested a review November 12, 2018 15:06
docker: &py36
- image: python:3.6
docker: &containers
- image: fishtownjacob/test-container
Copy link
Member

Choose a reason for hiding this comment

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

is this built by Dockerfile in this repo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I use this gross and lame shell script to do it:

#!/bin/bash
set -xeuo pipefail
commit_message="$1"
shift
if [[ -z ${commit_message} ]]; then
    echo 'no commit message'
    exit 1
fi
docker build --tag 'test-container:latest' .
container_id="$(docker run -it test-container:latest hostname | tr -d '[[:space:]]')"
docker commit -a 'Jacob Beck <jake@fishtownanalytics.com>' -m "${commit_message}" ${container_id} test-container
docker tag test-container:latest fishtownjacob/test-container:latest
docker push fishtownjacob/test-container:latest

@beckjake beckjake force-pushed the fix/dbt-deps-issues branch from af52ebf to 412b165 Compare November 12, 2018 18:29
@drewbanin
Copy link
Contributor

drewbanin commented Nov 13, 2018

This looks really great @beckjake!

Can you think of a good way to show a more obvious error message if git is not in the user's path? My guess is that if someone is just getting started with dbt and they don't have git installed on windows, this message might be a little confusing:

Could not find command, ensure it is in the user's PATH: "git"

Ideally, I think we'd show:

Could not find command, ensure it is in the user's PATH: "git"
Make sure git is installed on your machine. More information: https://docs.getdbt.com/docs/package-management

^ We can make a section about the requirements for dbt deps (including more info about how to install git) and link to that specific section.

I'm a little confused that you're seeing the Username/Password prompt here:

> dbt deps
Username for 'https://github.com': beckjake
Password for 'https://beckjake@github.com':
...

I couldn't reproduce that locally -- do you think it's a git version thing? Or something else?

I'll give this some spins on a Windows machine, but this is looking pretty good to me!

@beckjake
Copy link
Contributor Author

for the missing git error, I can easily change it to this:

Make sure git is installed on your machine. More information: https://docs.getdbt.com/docs/package-management
Encountered an error:
Could not find command, ensure it is in the user's PATH: "git"

Which is what you suggested but with order reversed. Is that ok?

I think the password prompt is a global git settings thing, or maybe version-dependent. it happens to me a lot when cloning a private repository from github via https urls. I've always taken it as a sign that my ssh key is somehow wrong.

@atharvai
Copy link

Drew asked me to test this out on Windows, here are the results: https://gist.github.com/atharvai/a302f1777a1f5b0202dffca937cb8de3

Summary: If the DBT_DOWNLOADS_DIR or default temp dir are on same drive as dbt project, then dbt deps is successful. Otherwise it fails on Access Denied on os.remove

@drewbanin
Copy link
Contributor

Thanks @atharvai! This is super helpful :)

@beckjake
Copy link
Contributor Author

With that most recent push I believe it is now fixed. I tested on Windows with an old drive I found, and cross drive moves from both temp directories and explicit DBT_DOWNLOADS_DIR settings are working as they should.

@atharvai
Copy link

atharvai commented Nov 14, 2018

I tested the latest commit and can confirm all is good!
One suggestion is to log if DBT_DOWNLOADS_DIR is set or not and value if set. It would help debugging.

Alternatively, set this log line to print full path

2018-11-14 18:06:46,187 (MainThread): Executing "git clone --depth 1 git@github.com:fishtown-analytics/dbt-utils.git 0a6a5f508a311be0d4ba05de66b74124"
...
2018-11-14 18:06:47,742 (MainThread): STDERR: "Cloning into '0a6a5f508a311be0d4ba05de66b74124'...

@drewbanin
Copy link
Contributor

@beckjake @atharvai awesome!! Really glad we got this working. Thanks for your insight @atharvai :)

This LGTM when the tests all pass!

@beckjake beckjake merged commit 735ff88 into dev/grace-kelly Nov 14, 2018
@beckjake beckjake deleted the fix/dbt-deps-issues branch November 14, 2018 20:31
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.

4 participants