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.

Sign up for GitHub

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

Jump to bottom

Validate require-dbt-version before validating dbt_project.yml schema #2638

Closed
clrcrl opened this issue Jul 21, 2020 · 1 comment · Fixed by #2726
Closed

Validate require-dbt-version before validating dbt_project.yml schema #2638

clrcrl opened this issue Jul 21, 2020 · 1 comment · Fixed by #2726
Labels
enhancement New feature or request good_first_issue Straightforward + self-contained changes, good for new contributors!
Milestone
Marian Anderson

Comments

@clrcrl
Copy link
Contributor

clrcrl commented Jul 21, 2020 •
edited
Loading

Describe the feature

In the rare occasion that we change the spec of dbt_project.yml, the errors around this can be pretty confusing. We should consider changing the order of validation to make this nicer.

Example

At the moment if you have:

required-dbt-version: ">0.17.1"
config-version: 2

And your run this with 0.16.1, you get back:

Encountered an error while reading the project:
  ERROR: Runtime Error
  at path []: Additional properties are not allowed ('config-version' was unexpected)
Encountered an error:
Runtime Error
  Could not run dbt

I think it would be a very small, but nice touch, if instead we checked the require-dbt-version first, and then the schema, so this is the error you get:

Running with dbt=0.16.1
Encountered an error while reading the project:
  ERROR: Runtime Error
  This version of dbt is not supported with the 'acme' package.
    Installed version of dbt: =0.16.1
    Required version of dbt for 'acme': ['>=0.17.1']
  Check the requirements for the 'acme' package, or run dbt again with --no-version-check

Encountered an error:
Runtime Error
  Could not run dbt

I feel that the second error is way easier to understand. Obviously not everyone will have a require-dbt-version config, but it’s a nice behavior for those that are doing the right thing 🙂

Describe alternatives you've considered

Doing nothing :)

Additional context

I think the config-version error comes from the initial read of dbt_project.yml, which precedes our ability to check the dbt version. So maybe a little gnarlier to do this than expected.

Who will this benefit?

Anyone working on dbt in a team, that needs to keep dbt versions in sync across many computers
@clrcrl clrcrl added enhancement New feature or request triage labels Jul 21, 2020
@beckjake beckjake added the good_first_issue Straightforward + self-contained changes, good for new contributors! label Jul 21, 2020
@jtcohen6 jtcohen6 removed the triage label Jul 21, 2020
@beckjake
Copy link
Contributor

This is a great idea, I've marked this as a "good first issue", but that doesn't mean "easy"! It's mostly a matter of pulling the various pieces together and should be pretty easy to test - it's going to work or it won't! I'm happy to help out with this if anyone wants to take it, or I will just do it whenever it's prioritized.

Here's my initial thoughts on how to solve it:

Currently, the process is:

  • load the project file as yaml
  • run the renderer over the loaded data
  • validate the schema of the rendered project file
  • parse the version
  • instantiate a project object
  • call validate() on it, which checks the --no-version-check CLI flag and conditionally validates that the versions match
    • on failure, include the project name in the message

We need to move version parsing and validation to be earlier, before we validate the file schema. One place I think would make a lot of sense for that is in Project.render_from_dict.

The nice thing about that is that at this point in the code, you can be sure that there's a name field (which you'll want for error messaging), and you have the rendered but not parsed/validated form of the required version field, if it exists.

You'll have to do some type checking, pull in the version-parsing from from_project_config (so _parse_versions), and write something that looks a lot like validate_version but isn't a method (you don't have a Project yet). I think those are the important parts.

The change also means that we can't just check --no-version-check like we do currently, but that's ok! Just move it into a flag in dbt.flags, it should basically do what USE_CACHE does.

@jtcohen6 jtcohen6 mentioned this issue Jul 28, 2020
Closed
@jtcohen6 jtcohen6 modified the milestones: 0.19.0, Marian Anderson Aug 5, 2020
@jtcohen6 jtcohen6 modified the milestones: Marian Anderson, 0.19.0 Aug 18, 2020
@beckjake beckjake mentioned this issue Aug 25, 2020
Merged
4 tasks
QMalcolm added a commit that referenced this issue May 29, 2024
@QMalcolm
Convert runtime version specifier tests to pytest tests and move to t…
18e4308 
…est_project

We've done two things in this commit, which arguably _should_ have been done in
two commits. First we moved the version specifier tests from `test_runtime.py::TestRuntimeConfig`
to `test_project.py::TestGetRequiredVersion` this is because what is really being
tested is the `_get_required_version` method. Doing it via `RuntimeConfig.from_parts` method
made actually testing it a lot harder as it requires setting up more of the world and
running with a _full_ project config dict.

The second thing we did was convert it from the old unittest implementation to a pytest
implementation. This saves us from having to create most of the world as we were doing
previously in these tests.

Of note, I did not move the test `test_unsupported_version_range_bad_config`. This test
is a bit different from the rest of the version specifier tests. It was introduced in
[1eb5857](1eb5857)
of [#2726](#2726) to resolve [#2638](#2638).
The focus of #2726 was to ensure the version specifier checks were run _before_ the validation
of the `dbt_project.yml`. Thus what this test is actually testing for is order of
operations at parse time. As such, this is really more a _functional_ test than a
unit test. In the next commit we'll get this test moved (and renamed)
QMalcolm added a commit that referenced this issue May 29, 2024
@QMalcolm
Convert runtime version specifier tests to pytest tests and move to t…
ede371e 
…est_project

We've done two things in this commit, which arguably _should_ have been done in
two commits. First we moved the version specifier tests from `test_runtime.py::TestRuntimeConfig`
to `test_project.py::TestGetRequiredVersion` this is because what is really being
tested is the `_get_required_version` method. Doing it via `RuntimeConfig.from_parts` method
made actually testing it a lot harder as it requires setting up more of the world and
running with a _full_ project config dict.

The second thing we did was convert it from the old unittest implementation to a pytest
implementation. This saves us from having to create most of the world as we were doing
previously in these tests.

Of note, I did not move the test `test_unsupported_version_range_bad_config`. This test
is a bit different from the rest of the version specifier tests. It was introduced in
[1eb5857](1eb5857)
of [#2726](#2726) to resolve [#2638](#2638).
The focus of #2726 was to ensure the version specifier checks were run _before_ the validation
of the `dbt_project.yml`. Thus what this test is actually testing for is order of
operations at parse time. As such, this is really more a _functional_ test than a
unit test. In the next commit we'll get this test moved (and renamed)
QMalcolm added a commit that referenced this issue May 30, 2024
@QMalcolm
Convert runtime version specifier tests to pytest tests and move to t…
aabe0ae 
…est_project

We've done two things in this commit, which arguably _should_ have been done in
two commits. First we moved the version specifier tests from `test_runtime.py::TestRuntimeConfig`
to `test_project.py::TestGetRequiredVersion` this is because what is really being
tested is the `_get_required_version` method. Doing it via `RuntimeConfig.from_parts` method
made actually testing it a lot harder as it requires setting up more of the world and
running with a _full_ project config dict.

The second thing we did was convert it from the old unittest implementation to a pytest
implementation. This saves us from having to create most of the world as we were doing
previously in these tests.

Of note, I did not move the test `test_unsupported_version_range_bad_config`. This test
is a bit different from the rest of the version specifier tests. It was introduced in
[1eb5857](1eb5857)
of [#2726](#2726) to resolve [#2638](#2638).
The focus of #2726 was to ensure the version specifier checks were run _before_ the validation
of the `dbt_project.yml`. Thus what this test is actually testing for is order of
operations at parse time. As such, this is really more a _functional_ test than a
unit test. In the next commit we'll get this test moved (and renamed)
QMalcolm added a commit that referenced this issue May 31, 2024
@QMalcolm
Create RuntimeConfig fixture (#10242)
a34267f 
* Create `runtime_config` fixture and necessary upstream fixtures

* Check for better scoped `ProjectContractError` in test_runtime tests

Previously in `test_unsupported_version_extra_config` and
`test_archive_not_allowed` we were checking for `DbtProjectError`. This
worked because `ProjectContractError` is a subclass of `DbtProjectError`.
However, if we check for `DbtProjectError` in these tests than, some tangential
failure which raises a `DbtProejctError` type error would go undetected. As
we plan on modifying these tests to be pytest in the coming commits, we want to
ensure that the tests are succeeding for the right reason.

* Convert `test_str` of `TestRuntimeConfig` to a pytest test using fixtures

* Convert `test_from_parts` of `TestRuntimeConfig` to a pytest test using fixtures

While converting `test_from_parts` I noticed the comment
>  TODO(jeb): Adapters must assert that quoting is populated?

This led me to beleive that `quoting` shouldn't be "fully" realized
in our project fixture unless we're saying that it's gone through
adapter instantiation. Thus I update the `quoting` on our project
fixture to be an empty dict. This change affected `test__str__` in
`test_project.py` which we thus needed to update accordingly.

* Convert runtime version specifier tests to pytest tests and move to test_project

We've done two things in this commit, which arguably _should_ have been done in
two commits. First we moved the version specifier tests from `test_runtime.py::TestRuntimeConfig`
to `test_project.py::TestGetRequiredVersion` this is because what is really being
tested is the `_get_required_version` method. Doing it via `RuntimeConfig.from_parts` method
made actually testing it a lot harder as it requires setting up more of the world and
running with a _full_ project config dict.

The second thing we did was convert it from the old unittest implementation to a pytest
implementation. This saves us from having to create most of the world as we were doing
previously in these tests.

Of note, I did not move the test `test_unsupported_version_range_bad_config`. This test
is a bit different from the rest of the version specifier tests. It was introduced in
[1eb5857](1eb5857)
of [#2726](#2726) to resolve [#2638](#2638).
The focus of #2726 was to ensure the version specifier checks were run _before_ the validation
of the `dbt_project.yml`. Thus what this test is actually testing for is order of
operations at parse time. As such, this is really more a _functional_ test than a
unit test. In the next commit we'll get this test moved (and renamed)

* Create a better test for checking that version checks come before project schema validation

* Convert `test_get_metadata` to pytest test

* Refactor `test_archive_not_allowed` to functional test

We do already have tests that ensure "extra" keys aren't allowed in
the dbt_project.yaml. This test is different because it's checking that
a specific key, `archive`, isn't allowed. We do this because at one point
in time `archive` _was_ an allowed key. Specifically, we stopped allowing
`archive` in dbt-core 0.15.0 via commit [f26948d](f26948d).
Given that it's been 5 years and a major version, we could probably remove
this test, but let's keep it around unless we start supporting `archive` again.

* Convert `warn_for_unused_resource_config_paths` tests to use pytest
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request good_first_issue Straightforward + self-contained changes, good for new contributors!
Projects
None yet
Milestone
Marian Anderson
Development

Successfully merging a pull request may close this issue.

Validate require-dbt-version before validating dbt_project.ymls chema dbt-labs/dbt-core
3 participants
@beckjake @jtcohen6 @clrcrl