Fix favor_state not used when merging manifest #1660
Conversation
WalkthroughThe update modifies the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DbtProject
participant dbt
User->>DbtProject: safe_parse_project()
DbtProject->>dbt: load manifest from artifact (dbt >= 1.8)
DbtProject->>dbt: merge_from_artifact(manifest, favor_state)
dbt-->>DbtProject: merged manifest
DbtProject-->>User: return result
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🔇 Additional comments (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue where the favor_state option was not being passed to the manifest merge process during defer in production projects.
- Passes self.favor_state to merge_from_artifact in dbt_core_integration.py
- Addresses behavior to correctly respect the favor_state during manifest merging
Comments suppressed due to low confidence (1)
dbt_core_integration.py:551
- Consider adding or updating tests to explicitly verify that the favor_state option is correctly respected when merging manifests.
favor_state=self.favor_state,
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 85ae7b9 in 35 seconds. Click for details.
- Reviewed
12lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. dbt_core_integration.py:548
- Draft comment:
Adding the favor_state parameter here fixes the issue with merging the manifest. Confirm the merge_from_artifact API accepts this keyword in this version branch. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to confirm that a specific API accepts a keyword in a certain version branch. This falls under asking the author to confirm their intention or to double-check things, which is against the rules.
Workflow ID: wflow_M7WLvhD5ho3yf977
You can customize 
| @@ -548,6 +548,7 @@ def safe_parse_project(self) -> None: | |||
| manifest = Manifest.from_writable_manifest(writable_manifest) | |||
| self.dbt.merge_from_artifact( | |||
| other=manifest, | |||
| favor_state=self.favor_state, | |||
There was a problem hiding this comment.
Signature of this function from dbt is:
# Called in GraphRunnableTask.before_run, RunTask.before_run, CloneTask.before_run
def merge_from_artifact(self, other: "Manifest") -> None:
Summary
Testing
npm test(fails: rimraf not found)npm ci(fails: npm fetch EHOSTUNREACH)Important
Fixes
safe_parse_project()indbt_core_integration.pyto respectfavor_statewhen merging manifests during defer.safe_parse_project()indbt_core_integration.pyto respectfavor_statewhen merging production manifest during defer.npm testfails due to missingrimraf.npm cifails due toEHOSTUNREACHerror.This description was created by
for 85ae7b9. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit