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

[Backport 1.7.latest] Support hierarchical config setting for SavedQueryExport configs #9074

Merged
merged 1 commit into from
Nov 15, 2023

Conversation

github-actions[bot]
Copy link
Contributor

Backport c2f7d75 from #9065.

@github-actions github-actions bot requested a review from a team as a code owner November 14, 2023 19:14
@github-actions github-actions bot requested a review from ChenyuLInx November 14, 2023 19:14
@cla-bot cla-bot bot added the cla:yes label Nov 14, 2023
@QMalcolm QMalcolm force-pushed the backport-9065-to-1.7.latest branch from dfbad6f to 313c957 Compare November 15, 2023 20:49
Copy link

codecov bot commented Nov 15, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2a9c368) 86.59% compared to head (1012673) 86.54%.

Additional details and impacted files
@@              Coverage Diff               @@
##           1.7.latest    #9074      +/-   ##
==============================================
- Coverage       86.59%   86.54%   -0.06%     
==============================================
  Files             179      179              
  Lines           26541    26542       +1     
==============================================
- Hits            22984    22971      -13     
- Misses           3557     3571      +14     
Flag Coverage Δ
integration 83.36% <100.00%> (-0.13%) ⬇️
unit 64.77% <30.76%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

* Add test asserting `SavedQuery` configs can be set from `dbt_project.yml`

* Allow extraneous properties in Export configs

This brings the Export config object more in line with how other config
objects are specified in the unparsed definition. It allows for specifying
of extra configs, although they won't get propagate to the final config.

* Add `ExportConfig` options to `SavedQueryConfig` options

This allows for specifying `ExportConfig` options at the `SavedQueryConfig` level.
This also therefore allows these options to be specified in the dbt_project.yml
config. The plan in the follow up commit is to merge the `SavedQueryConfig` options
into all configs of `Exports` belonging to the saved query.

There are a couple caveots to call out:
1. We've used `schema` instead of `schema_name` on the `SavedQueryConfig` despite
it being called `schema_name` on the `ExportConfig`. This is because need `schema_name`
to be the name of the property on the `ExportConfig`, but `schema` is the user facing
specification.
2. We didn't add the `ExportConfig` `alias` property to the `SavedQueryConfig` This
is because `alias` will always be specific to a single export, and thus it doesn't
make sense to allow defining it on the `SavedQueryConfig` to then apply to all
`Exports` belonging to the `SavedQuery`

* Begin inheriting configs from saved query config, and transitively from project config

Export configs will now inherit from saved query configs, with a preference
for export config specifications. That is to say an export config will inherity
a config attr from the saved query config only if a value hasn't been supplied
on the export config directly. Additionally because the saved query config has
a similar relationship with the project config, exports configs can inherit
from the project config (again with a preference for export config specifications).

* Correct conditional in export config building for map schema to schema_name

I somehow wrote a really weird, but also valid, conditional statement. Previously
the conditional was
```
if combined.get("schema") is not combined.get("schema_name") is None:
```
which basically checked whether `schema` was a boolean that didn't match
the boolean of whether `schema_name` was None. This would pretty much
always evaluate to True because `schema` should be a string or none, not
a bool, and thus would never match the right hand side. Crazy. It has now
been fixed to do the thing we want to it to do. If `schema` isn't `None`,
and `schema_name` is `None`, then set `schema_name` to have the value of
`schema`.

* Update parameter names in `_get_export_config` to be more verbose

(cherry picked from commit c2f7d75)
@QMalcolm QMalcolm force-pushed the backport-9065-to-1.7.latest branch from 313c957 to 1012673 Compare November 15, 2023 23:08
@QMalcolm QMalcolm added cla:yes and removed cla:yes labels Nov 15, 2023
@QMalcolm
Copy link
Contributor

@cla-bot[bot] check

Copy link

cla-bot bot commented Nov 15, 2023

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

@QMalcolm QMalcolm merged commit 73ebe53 into 1.7.latest Nov 15, 2023
100 checks passed
@QMalcolm QMalcolm deleted the backport-9065-to-1.7.latest branch November 15, 2023 23:34
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.

2 participants