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

do not try to remove remote after dbt init #1210

Merged
merged 2 commits into from
Jan 4, 2019

Conversation

drewbanin
Copy link
Contributor

Fixes #1209 by removing a call to delete the remote from the cloned repo. The entire .git dir is deleted right before this, so the command previously could not succeed

Copy link
Contributor

@beckjake beckjake left a comment

Choose a reason for hiding this comment

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

some style-ish comments, lgtm with or without them

return "test/integration/040_init_test/models"

@use_profile('postgres')
def test_init_task(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

if you put _postgres_ in the name here you will avoid a UserWarning.

if os.path.exists(project_name):
shutil.rmtree(project_name)

DBTIntegrationTest.tearDown(self)
Copy link
Contributor

Choose a reason for hiding this comment

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

super(TestInit, self).tearDown() is the canonical syntax for this, though it isn't very important here.

project_file_exists = os.path.exists(project_file)

self.assertTrue(dir_exists)
self.assertTrue(project_file_exists)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also assert that there is no .git?

@drewbanin
Copy link
Contributor Author

@beckjake just made some changes per your feedback. Will merge when the tests pass

@drewbanin drewbanin merged commit 3c25a9b into dev/grace-kelly Jan 4, 2019
@drewbanin drewbanin deleted the fix/dbt-init-command branch January 4, 2019 19:56
@hoanghapham
Copy link

hoanghapham commented Jan 8, 2019

@drewbanin Today I encountered the same problem, and I've just upgraded dbt but remote link is still removed from my cloned repo.

error:

 “pip install dbt --upgrade” terminated by signal SIGSEGV (Address boundary error)

@drewbanin
Copy link
Contributor Author

hey @hoanghapham - this PR fixed a bug wherein dbt would try to remove the remote after deleting the .git directory. Since we had already deleted .git, the subsequent call to git remote rm origin (obviously) failed. So, this PR removed the call to remove the origin, but maintained the code to delete the .git directory.

I'm not so sure I understand the pip install error you're seeing. Is that related to dbt init? Or something else?

@hoanghapham
Copy link

oh ok, actually I got that error when I tried to upgrade dbt today.
So actually removing .git folder when running dbt init is an intended behavior? If yes, why is that?

@drewbanin
Copy link
Contributor Author

Hey @hoanghapham - we do this so new users start with a clean slate. If we didn't remove the .git directory, users would need to manually do it. Further, I like the idea of cleaning up the git history. It would be strange to me if dbt init gave me back a repo with a history like this: https://github.com/fishtown-analytics/dbt-starter-project/commits/master

If you have suggestions for how dbt init should work, please feel free to open a new issue

@hoanghapham
Copy link

Thanks. I have a master analytics repository where I put different dbt projects into, so I don't really want to reset everything every time I init a new project inside the analytics directory. Not sure if this approach makes sense to you.

I think this behavior should also be mentioned in the docs as you encourage using version control in analytics.

@drewbanin
Copy link
Contributor Author

@hoanghapham are you saying that dbt is removing the remote of your root project? I think that was the behavior of the 0.12.2rc1 release, which was fixed by this PR! I was able to replicate the behavior you described with 0.12.2rc1, but not with 0.12.2.

@hoanghapham
Copy link

hoanghapham commented Jan 8, 2019

removing the remote of your root project

ah yes, that's the problem, sorry for not being specific.
again, I encountered this error when running pip install dbt --upgrade via fish.

pip install dbt --upgrade” terminated by signal SIGSEGV (Address boundary error)

However, pip3 install dbt --upgrade works fine @drewbanin

@drewbanin
Copy link
Contributor Author

yeah, no prob! I just tried out pip install dbt --upgrade with python2 and it worked ok for me. There are a ton of things that could be happening here, but I've never seen this particular error before. dbt does support python2 and python3, so this would ideally work. Can you make a new issue with some info about your python and your machine? Would be happy to check it out

@hoanghapham
Copy link

Thanks @drewbanin , will do tomorrow. Please let me know what kind of info you will need.

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