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

Update UnsetProfileConfig.from_args to use args.project_dir if passed #2339

Conversation

nickwu241
Copy link
Contributor

@nickwu241 nickwu241 commented Apr 17, 2020

resolves #2338

Description

Code Change
If dbt deps specifies --project-dir, use that as the project_root rather than os.getcwd().

Local Testing
Made sure both with and without --project-dir still works.

Prior to my changes:

❯ dbt deps --project-dir project
Running with dbt=0.17.0-a1
Encountered an error while reading the project:
  ERROR: Runtime Error
  no dbt_project.yml found at expected path /Users/nickwu/dbt-bug/dbt_project.yml
Encountered an error:
Runtime Error
  Could not run dbt

After my changes:

❯ dbt deps --project-dir project
Running with dbt=0.17.0-a1
Installing https://github.com/fishtown-analytics/[email protected]
  Installed from revision 0.1.21

❯ cd project
❯ dbt deps
Running with dbt=0.17.0-a1
Installing https://github.com/fishtown-analytics/[email protected]
  Installed from revision 0.1.21

Testing
Q: I'm not sure where's the best place to add this type of integration test, could someone point me in the right direction? Thank you!

Note
Although I do see a move_to_nearest_project_dir used by class ConfiguredTask(BaseTask) which will probably handle this. But dbt deps doesn't make use of it because it inherits from BaseTask not ConfiguredTask
Maybe we want to make use of that move_to_nearest_project_dir?

Saw that it was changed so that dbt deps doesn't need a profile which is 👍 42e8c56

CLA

I've signed the Individual Contributor License Agreement v1.0 linked but the bot is asking me to sign it.

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 to the "dbt next" section.

@cla-bot
Copy link

cla-bot bot commented Apr 17, 2020

Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA.

In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, don't hesitate to ping @drewbanin.

CLA has not been signed by users: @nickwu241

@cla-bot
Copy link

cla-bot bot commented Apr 17, 2020

Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA.

In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, don't hesitate to ping @drewbanin.

CLA has not been signed by users: @nickwu241

@cla-bot
Copy link

cla-bot bot commented Apr 17, 2020

Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA.

In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, don't hesitate to ping @drewbanin.

CLA has not been signed by users: @nickwu241

@beckjake
Copy link
Contributor

@cla-bot check

@cla-bot cla-bot bot added the cla:yes label Apr 17, 2020
@cla-bot
Copy link

cla-bot bot commented Apr 17, 2020

The cla-bot has been summoned, and re-checked this pull request!

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.

Hi @nickwu241, thanks for your contribution! This is a good fix, there's one thing that's making our linting tool upset but otherwise this looks great.

I've signed the Individual Contributor License Agreement v1.0 linked but the bot is asking me to sign it.

There's a manual step where we have to edit a git repository, all set now.

I see this means that the test we currently have for this feature is meaningless, which is concerning. We probably should come up with a way to test that we're using the given profile directory in deps - maybe we can override the profile to exist but be invalid for that test?

core/dbt/config/runtime.py Outdated Show resolved Hide resolved
@nickwu241
Copy link
Contributor Author

nickwu241 commented Apr 17, 2020

There's a manual step where we have to edit a git repository, all set now.

Thanks!

I see this means that the test we currently have for this feature is meaningless, which is concerning. We probably should come up with a way to test that we're using the given profile directory in deps - maybe we can override the profile to exist but be invalid for that test?

Is the current test https://github.com/fishtown-analytics/dbt/blob/42e8c56b133be901cd22d67ba94d678bf2b06970/test/integration/006_simple_dependency_test/test_simple_dependency.py#L230

class TestSimpleDependencyNoProfile(TestSimpleDependency):
    def run_deps(self):
        tmpdir = tempfile.mkdtemp()
        try:
            result = self.run_dbt(["deps", "--profiles-dir", tmpdir])
        finally:
            shutil.rmtree(tmpdir)
        return result

If so, it looks like it uses tests using --profile-dirs which makes the profile empty so it's probably testing correctly? But there are currently no test for running similar to dbt deps --project-dir outside the directory with the dbt_project.yml.

@beckjake
Copy link
Contributor

Ah, you're right, I was thinking of that --profiles-dir test! We should add a new one for --project-dir... but we should probably just add tests in general for it. I think it would be enough to have a test that:

  • gets the starting directory path
  • makes an empty directory and cd into it
  • runs each of a basic 'dbt deps', 'dbt seed', 'dbt run', and 'dbt test' with --project-dir` pointing to where the test started

Are you up for that? I would just hate to regress here - it feels like we will easily break this by accident in the future.

@nickwu241
Copy link
Contributor Author

nickwu241 commented Apr 17, 2020

Ah, you're right, I was thinking of that --profiles-dir test! We should add a new one for --project-dir... but we should probably just add tests in general for it. I think it would be enough to have a test that:

  • gets the starting directory path
  • makes an empty directory and cd into it
  • runs each of a basic 'dbt deps', 'dbt seed', 'dbt run', and 'dbt test' with --project-dir` pointing to where the test started

Are you up for that? I would just hate to regress here - it feels like we will easily break this by accident in the future.

Yeah for sure +1 for the preventing regression!

I have something similar to the test you've written

class TestSimpleDependencyWithProjectDirSet(TestSimpleDependency):
    def run_deps(self):
        workdir = os.getcwd()
        tmpdir = tempfile.mkdtemp()
        os.chdir(tmpdir)
        try:
            result = self.run_dbt(["deps", "--project-dir", workdir])
        finally:
            shutil.rmtree(tmpdir)
        return result

But the new test is failing when I make test-quick, I'm trying to fix it.
Q: Is there a way to run only 1 set of integration tests?

@beckjake
Copy link
Contributor

beckjake commented Apr 17, 2020

But the new test is failing when I make test-quick, I'm trying to fix it.
Q: Is there a way to run only 1 set of integration tests?

I run this (via a script!):
docker-compose run --rm test tox -e explicit-py36 -- -s -k TestSimpleDependencyWithProjectDirSet -m profile_postgres test/integration/006_simple_dependency_test/test_simple_dependency.py

There is probably a better way to do it.

@beckjake
Copy link
Contributor

beckjake commented Apr 17, 2020

Also, looking over your code sample very quickly: I would guess that you might have to add a os.chdir(workdir) before you shutil.rmtree.

@nickwu241
Copy link
Contributor Author

nickwu241 commented Apr 17, 2020

So since we want to test all the simple commands, I added it to 015_cli_invocation_tests/test_cli_invocation.py.

Please let me know if there are any issues / styling guidelines, etc! Thanks for the pointer and also the command, that's handy :)

  1. I verified it fails before my changes
failing test logs prior to changes
test/integration/015_cli_invocation_tests/test_cli_invocation.py::TestCLIInvocationWithProjectDir::test_postgres_dbt_commands_with_randomdir_as_project_dir
test/integration/015_cli_invocation_tests/test_cli_invocation.py::TestCLIInvocationWithProjectDir::test_postgres_dbt_commands_with_cwd_as_project_dir
[gw0] PASSED test/integration/015_cli_invocation_tests/test_cli_invocation.py::TestCLIInvocationWithProjectDir::test_postgres_dbt_commands_with_cwd_as_project_dir
[gw1] FAILED test/integration/015_cli_invocation_tests/test_cli_invocation.py::TestCLIInvocationWithProjectDir::test_postgres_dbt_commands_with_randomdir_as_project_dir

============================================== FAILURES ===============================================
______ TestCLIInvocationWithProjectDir.test_postgres_dbt_commands_with_randomdir_as_project_dir _______
[gw1] linux -- Python 3.6.9 /usr/app/.tox/explicit-py36/bin/python

self = <test_cli_invocation.TestCLIInvocationWithProjectDir testMethod=test_postgres_dbt_commands_with_randomdir_as_project_dir>

    @use_profile('postgres')
    def test_postgres_dbt_commands_with_randomdir_as_project_dir(self):
        workdir = os.getcwd()
        with tempfile.TemporaryDirectory() as tmpdir:
            os.chdir(tmpdir)
>           self._run_simple_dbt_commands(workdir)

test/integration/015_cli_invocation_tests/test_cli_invocation.py:136:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
test/integration/015_cli_invocation_tests/test_cli_invocation.py:139: in _run_simple_dbt_commands
    self.run_dbt(['deps', '--project-dir', project_dir])
test/integration/base.py:533: in run_dbt
    res, success = self.run_dbt_and_check(args=args, strict=strict, parser=parser, profiles_dir=profiles_dir)
test/integration/base.py:578: in run_dbt_and_check
    return dbt.handle_and_check(final_args)
core/dbt/main.py:159: in handle_and_check
    task, res = run_from_args(parsed)
core/dbt/main.py:198: in run_from_args
    task = parsed.cls.from_args(args=parsed)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

cls = <class 'dbt.task.deps.DepsTask'>
args = Namespace(cls=<class 'dbt.task.deps.DepsTask'>, debug=False, log_cache_events=True, log_format='default', partial_pars...ct=True, target=None, test_new_parser=True, use_cache=True, vars='{}', warn_error=False, which='deps', write_json=True)

    @classmethod
    def from_args(cls, args):
        try:
            config = cls.ConfigType.from_args(args)
        except dbt.exceptions.DbtProjectError as exc:
            logger.error("Encountered an error while reading the project:")
            logger.error("  ERROR: {}".format(str(exc)))

            tracking.track_invalid_invocation(
                args=args,
                result_type=exc.result_type)
>           raise dbt.exceptions.RuntimeException('Could not run dbt') from exc
E           dbt.exceptions.RuntimeException: Runtime Error
E             Could not run dbt

core/dbt/task/base.py:67: RuntimeException
---------------------------------------- Captured logbook call ----------------------------------------
[DEBUG] dbt: Acquiring new postgres connection "__test".
[DEBUG] dbt: Opening a new connection, currently in state init
[DEBUG] dbt: Acquiring new postgres connection "__test".
[DEBUG] dbt: Opening a new connection, currently in state init
[DEBUG] dbt: test connection "__test" executing: DROP SCHEMA IF EXISTS "test15871431688537_test_cli_invocation_015" CASCADE
[DEBUG] dbt: test connection "__test" executing: CREATE SCHEMA "test15871431688537_test_cli_invocation_015"
[INFO] dbt: Invoking dbt with ['--strict', '--test-new-parser', 'deps', '--project-dir', '/tmp/dbt-int-test-eo4fscjs', '--profiles-dir', '/tmp/dbt-int-test-eo4fscjs', '--log-cache-events']
[DEBUG] dbt: Acquiring new postgres connection "__test".
[DEBUG] dbt: Opening a new connection, currently in state init
[DEBUG] dbt: Acquiring new postgres connection "__test".
[DEBUG] dbt: Opening a new connection, currently in state init
[DEBUG] dbt: test connection "__test" executing: DROP SCHEMA IF EXISTS "test15871431688537_test_cli_invocation_015" CASCADE
[DEBUG] dbt: Connection '__test' was left open.
[DEBUG] dbt: On __test: Close
======================================= slowest test durations ========================================
4.71s call     test/integration/015_cli_invocation_tests/test_cli_invocation.py::TestCLIInvocationWithProjectDir::test_postgres_dbt_commands_with_cwd_as_project_dir
0.40s call     test/integration/015_cli_invocation_tests/test_cli_invocation.py::TestCLIInvocationWithProjectDir::test_postgres_dbt_commands_with_randomdir_as_project_dir

(0.00 durations hidden.  Use -vv to show these durations.)
================================= 1 failed, 1 passed in 32.34 seconds =================================
ERROR: InvocationError for command /bin/bash -c '/usr/app/.tox/explicit-py36/bin/python -m pytest --durations 0 -v -s -k TestCLIInvocationWithProjectDir -n auto -m profile_postgres test/integration/015_cli_invocation_tests/test_cli_invocation.py' (exited with code 1)
_______________________________________________ summary _______________________________________________
ERROR:   explicit-py36: commands failed
  1. Then made my changes in this PR, ran it locally and it passes :) which hopefully we can see by CI as well.

@nickwu241 nickwu241 requested a review from beckjake April 17, 2020 17:12
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.

This is great! I have one Windows-specific comment, but everything else looks good to me.

While our core code is strict on linting, the integration tests aren't linted, so don't worry about formatting too much there. Very readable and that's all I care about, at least!

@use_profile('postgres')
def test_postgres_dbt_commands_with_randomdir_as_project_dir(self):
workdir = os.getcwd()
with tempfile.TemporaryDirectory() as tmpdir:
Copy link
Contributor

Choose a reason for hiding this comment

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

What!? How did I not know about this context manager? This is awesome!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha I found out about it today!

workdir = os.getcwd()
with tempfile.TemporaryDirectory() as tmpdir:
os.chdir(tmpdir)
self._run_simple_dbt_commands(workdir)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we will need an os.chdir(workdir) after this line for Windows tests. While linux/osx will happily rm -r the directory you're in for the most part, Windows gets cranky about it.

Copy link
Contributor Author

@nickwu241 nickwu241 Apr 17, 2020

Choose a reason for hiding this comment

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

That makes sense, changes done.

and also curious: would this be caught in tests (i.e. does dbt run tests w/ windows)?

Copy link
Contributor

@beckjake beckjake Apr 17, 2020

Choose a reason for hiding this comment

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

Yup! I'm about to kick them off now, I just didn't want to run the full test suite and then have to re-run later. Running tests for external contributors isn't quite as friction-free as we'd like yet.

https://dev.azure.com/fishtown-analytics/dbt/_build/results?buildId=1798&view=results - it's the blue rocket ship link in the "checks" section

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see HAHA, that's awesome, I assume it's a lot of resources to use to run all these tests 😛 Appreciate it.

@beckjake beckjake merged commit c05b45b into dbt-labs:dev/octavius-catto Apr 17, 2020
@beckjake
Copy link
Contributor

Thanks so much for contributing to dbt @nickwu241! This will go out with 0.17.0.

@nickwu241 nickwu241 deleted the fix/0.16.1/UnsetProfileConfig-use-args-project-dir branch April 17, 2020 22:02
@nickwu241
Copy link
Contributor Author

nickwu241 commented Apr 17, 2020

No problem, thank you for all the help, appreciate it! 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v0.16.1: dbt deps doesn't respect "--project-dir" flag
2 participants