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 9328 to 1.7.latest #9391

Merged
merged 3 commits into from
Jan 17, 2024
Merged

Conversation

ChenyuLInx
Copy link
Contributor

@ChenyuLInx ChenyuLInx commented Jan 16, 2024

resolves #9327 and #9112

Co-authored-by: Peter Allen Webb <peter.webb@dbtlabs.com>
(cherry picked from commit 1e4286a)
@ChenyuLInx ChenyuLInx requested review from a team as code owners January 16, 2024 22:41
@ChenyuLInx ChenyuLInx requested review from jzhu13 and peterallenwebb and removed request for a team January 16, 2024 22:41
@cla-bot cla-bot bot added the cla:yes label Jan 16, 2024
@ChenyuLInx ChenyuLInx changed the base branch from main to 1.7.latest January 16, 2024 22:42
Copy link

codecov bot commented Jan 16, 2024

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (d338b3e) 86.67% compared to head (9602d3c) 86.62%.

Files Patch % Lines
core/dbt/contracts/state.py 66.66% 3 Missing ⚠️
core/dbt/parser/manifest.py 90.00% 1 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff               @@
##           1.7.latest    #9391      +/-   ##
==============================================
- Coverage       86.67%   86.62%   -0.05%     
==============================================
  Files             179      179              
  Lines           26612    26626      +14     
==============================================
- Hits            23065    23064       -1     
- Misses           3547     3562      +15     
Flag Coverage Δ
integration 83.44% <91.66%> (-0.14%) ⬇️
unit 64.97% <35.41%> (-0.01%) ⬇️

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.

@@ -264,23 +265,12 @@ def wrapper(*args, **kwargs):
raise DbtProjectError("profile, project, and runtime_config required for manifest")

runtime_config = ctx.obj["runtime_config"]
register_adapter(runtime_config)

Copy link
Contributor

Choose a reason for hiding this comment

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

Re-locating the call to register_adapter() makes me nervous. With this change, it looks like register_adapter() was previously called even when ctx.obj.get("manifest) was already set. But now, it will not be called in that scenario. Do you know if that is safe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it is still being called in line 271 in this fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, but it is only called if the if condition evaluates to True. What about the case where it is False?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! Update to restore previous behavior

@ChenyuLInx ChenyuLInx merged commit 6e33183 into 1.7.latest Jan 17, 2024
96 checks passed
@ChenyuLInx ChenyuLInx deleted the backport-9328-to-1.7.latest branch January 17, 2024 21:27
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.

3 participants