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

dbt deps does not handle non-master default branch #3057

Closed
clrcrl opened this issue Feb 8, 2021 · 7 comments · Fixed by #3104
Closed

dbt deps does not handle non-master default branch #3057

clrcrl opened this issue Feb 8, 2021 · 7 comments · Fixed by #3104
Labels
bug Something isn't working good_first_issue Straightforward + self-contained changes, good for new contributors!

Comments

@clrcrl
Copy link
Contributor

clrcrl commented Feb 8, 2021

Steps to reproduce

  1. Add a package using a the git syntax. The package must use a default branch other than master. In this example, the default branch is main (this package is currently an internal package but will be public soon)
$ cat packages.yml
packages:
  - git: git@github.com:fishtown-analytics/dbt-artifacts.git
    
  1. Run dbt deps
$ dbt deps
Running with dbt=0.19.0
Encountered an error:
Error checking out spec='master' for repo git@github.com:fishtown-analytics/dbt-artifacts.git
fatal: couldn't find remote ref master

Expected behavior

Package should install from the default branch, main and warn about an unpinned package

If I do specify revision: main, I also expect to get an unpinned error, however currently no warning is shown.

dbt version

0.19.0 :)

Additional context

Relevant code

Going to be more common since GitHub updated their default branch name.

@clrcrl clrcrl added bug Something isn't working triage labels Feb 8, 2021
@jtcohen6
Copy link
Contributor

jtcohen6 commented Feb 8, 2021

Good call :)

@jtcohen6 jtcohen6 added good_first_issue Straightforward + self-contained changes, good for new contributors! and removed triage labels Feb 8, 2021
@VasiliiSurov
Copy link
Contributor

@jtcohen6 , @clrcrl
I want to pick up this task if nobody minds

Here is my ideas:

  1. Add new method to client/git.py default_branch(repo) that will get the value of default branch from the git configuration using CLI git remote show [repository]
  2. Modify deps/git.py@resolved
    instead of hard coded 'master' assign result value from default_branch
    https://github.com/fishtown-analytics/dbt/blob/c8f0469a44f84cfc7d48cb6b938c6cd27288feba/core/dbt/deps/git.py#L136
  3. Modify deps/git.py@_fetch_metadata
    check self.revision for ('master', 'main')
    https://github.com/fishtown-analytics/dbt/blob/c8f0469a44f84cfc7d48cb6b938c6cd27288feba/core/dbt/deps/git.py#L75

And one more possible change that is out of the scope but is related

  1. Modify clients/git.py@checkout
    instead of hard coded 'master' assign result value from the default_branch
    https://github.com/fishtown-analytics/dbt/blob/c8f0469a44f84cfc7d48cb6b938c6cd27288feba/core/dbt/clients/git.py#L55

@jtcohen6
Copy link
Contributor

@VasiliiSurov Thanks for your interest! We'd welcome a contribution for this, and your proposal sounds solid.

I imagine we may also wish to update the unit test here:

https://github.com/fishtown-analytics/dbt/blob/c8f0469a44f84cfc7d48cb6b938c6cd27288feba/test/unit/test_deps.py#L95-L109

Let me know if I can be of any help :)

@kwigley
Copy link
Contributor

kwigley commented Feb 12, 2021

💡 just jotting down my thoughts here, I haven't tested this out. I know this applies to GitHub: after running git clone the default branch defined in GitHub will be checked out locally. When there is no version supplied for a package in packages.yml, we should probably:

  • after cloning the package, either skip checking out anything or check out the current branch (maybe even use HEAD here?)
  • ensure unpinned warning is displayed

There should be no reference to master after these changed.

When the version supplied for a package is a branch name, we could:

  • check if the version points to a branch as opposed to a tag and display an appropriate warning.

I apologize if this is repeating or different than was is mentioned above! I think the main issue here is git has no notion of default branch, so git clone is the important piece to get this info from.

@VasiliiSurov
Copy link
Contributor

@kwigley If I correctly understood your idea we can achieve it by simple replacing 'master' to 'HEAD' in the same 3 lines I mentioned before and no need to jump through hoops to get the name of the default branch.

@VasiliiSurov
Copy link
Contributor

image

@kwigley
Copy link
Contributor

kwigley commented Feb 15, 2021

@VasiliiSurov I think so! Don't quote me 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good_first_issue Straightforward + self-contained changes, good for new contributors!
Projects
None yet
4 participants