fix: resolve DBT_PROFILES_DIR correctly if relative path specified
#1573
Conversation
WalkthroughThis pull request modifies the Changes
Assessment against linked issues
Possibly related PRs
Tip 🌐 Web search-backed reviews and chat
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to d3864a8 in 56 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. dbt_core_integration.py:220
- Draft comment:
Nice fix. The unnecessary join with project_dir has been removed for relative DBT_PROFILES_DIR paths. Consider simplifying further by returning os.path.normpath(profiles_dir) directly, since os.path.join with a single argument is redundant. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_Dpk2RzYw2qB4JPf1
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
diegoquintanav
changed the title
DBT_PROFILES_DIR correctly if relative path specified
|
This change will break things for other people.For the extension the CWD is not obvously set, compared to CLI where there is clear concept of CWD. Becasue of that we went with choosing the project directory as the CWD. |
|
@diegoquintanav I am not sure how this resolves the original bug #1554 . Can you help explain? |
Hey thanks for checking this PR! This change affects only the scenario where DBT_PROFILES_DIR is set as an environment variable and is not an absolute path. I've pasted my original reply to the issue in the PR description if that helps. In what cases would you say it breaks behaviour for other people? |
|
In the code that is removed in this PR the path is relative to the project root. In your case it is to be defined, by the calling code (probably the first workspace root directory). All people that have setup a relative path from the project root would have an issue if we merge this. |
thanks for the follow-up @mdesmet, I'm not sure if I understand the scenario you present. My assumptions are that
In this scenario, what would you say is the value of |
We don't use this environment variable as it is not compatible with multi dbt project projects, but we can rethink that one.
I'm open to rethink this but in that case we probably should set the project dir to what is in |
|
Thanks @mdesmet for your input. If I set DBT_PROFILES_DIR=./profiles/ DBT_PROJECT_DIR=./mydbtproject/ dbt ls -m fooWould break (there's no file at
I think this is key. What is a multi-dbt project? does a hacky solution can be done, if the extension decides to use native if "DBT_PROFILES_DIR" in os.environ:
# case when DBT_PROFILES_DIR=~/something
profiles_dir = os.path.expanduser(os.environ["DBT_PROFILES_DIR"])
if os.path.isabs(profiles_dir):
# if path is absolute, then ok, go ahead
return os.path.normpath(profiles_dir)
if "DBT_PROJECT_DIR" in os.environ: # new
# assume DBT_PROFILES_DIR is relative to the same directory that DBT_PROJECT_DIR was set to
# return the profiles_dir path as it is
return os.path.normpath(profiles_dir)) # new
return os.path.normpath(os.path.join(project_dir, profiles_dir))a different approach perhaps would be that if vscode-dbt-power-user/src/manifest/dbtWorkspaceFolder.ts Lines 87 to 90 in 5735fa6 resolve the argument passed to |
|
@mdesmet @saravmajestic Can you please review the code? |
This can work but I would add a check if the path exists and then only go in this path, this will act as an additional safeguard.
This won't resolve the issue, as still the profiles dir will be evaluated against the project dir. |
3 participants
Overview
Problem
if
DBT_PROFILES_DIRis set, thenvscode-dbt-power-user/dbt_core_integration.py
Line 220 in 5735fa6
is joining
project_dirwithprofiles_dir. Ifproject_diris coming fromDBT_PROJECT_DIR, and both paths are relative then this is effectively producing<DBT_PROJECT_DIR>/<DBT_PROFILES_DIR>which may not exist.In my case, I have
and this method is returning
default_profiles_dir=my_project/my_project/my_profilesThe value of
project_dirbeing passed is resolved as**/dbt_project.ymlas invscode-dbt-power-user/src/manifest/dbtWorkspaceFolder.ts
Line 90 in 5735fa6
see #1554 (comment) for the original reply
Closes #1554
Solution
Removing the unnecessary path join
Screenshot/Demo
A picture is worth a thousand words. Please highlight the changes if applicable.
How to test
Checklist
README.mdupdated and added information about my changeImportant
Fixes path resolution in
default_profiles_dir()by removing unnecessary path joining withproject_dirwhenDBT_PROFILES_DIRis set.default_profiles_dir()indbt_core_integration.pyby removing unnecessaryos.path.joinwithproject_dirwhenDBT_PROFILES_DIRis set.DBT_PROFILES_DIRis used as is if it's an absolute path, or resolved correctly if relative.This description was created by
for d3864a8. It will automatically update as commits are pushed.
Summary by CodeRabbit