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

Default to current working directory for profiles.yml and fall back to ~/.dbt #5412

Closed
wants to merge 3 commits into from

Conversation

dbeatty10
Copy link
Contributor

@dbeatty10 dbeatty10 commented Jun 27, 2022

resolves #5411

Description

Default to current working directory for profiles.yml and fall back to ~/.dbt.

Various alternatives and trade-offs discussed in #5411.

Checklist

@cla-bot cla-bot bot added the cla:yes label Jun 27, 2022
@github-actions
Copy link
Contributor

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

@jtcohen6 jtcohen6 added Team:Language ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering labels Jul 21, 2022
@jtcohen6
Copy link
Contributor

jtcohen6 commented Jul 21, 2022

Conceptually, good by me!

This section of the codebase is tricky (along with all config initialization). We'll definitely want some tests for this behavior as well. I marked ready_for_review, and I tagged in Language, though this is a fuzzy boundary between Language/Execution today I've tagged in both Language + Execution. Either/both should be able to help out here.

@ChenyuLInx
Copy link
Contributor

@dbeatty10 This unittest here will write a profile.yml to local and test things, I think you can maybe follow things here to add an unittest for your change. Happy to pair a bit to figure out exactly how

@dbeatty10
Copy link
Contributor Author

Thank you for pointing me to that test @ChenyuLInx ! 👍

I will give it a shot and let you know how it goes.

@leahwicz
Copy link
Contributor

@dbeatty10 did we want to get this in for the 1.3 RC? I just want to make sure we plan to get this in if so

@dbeatty10
Copy link
Contributor Author

@leahwicz Yeah, it would be awesome to get this into the 1.3 RC!

@jtcohen6 would you be willing to give this a look? Specifically, how would we tell if it works correctly with dbt Cloud and dbt Server?

Good news:

Less fun news:

  • our current implementation and behavior is mainly driven by flags.PROFILES_DIR which is set pretty much before anything else in the code base is executed. Our testing framework runs after this is set, so would need major re-factoring of flags.PROFILES_DIR, etc to make it more testable.

Here's the easiest way to confirm the desired behavior manually:

# Activate a virtual environment and install this branch

# Using profiles.yml file at $HOME/.dbt/profiles.yml
dbt debug

touch profiles.yml

# Using profiles.yml file at $(pwd)/profiles.yml
dbt debug

@dbeatty10 dbeatty10 marked this pull request as ready for review September 16, 2022 04:07
@dbeatty10 dbeatty10 requested a review from a team September 16, 2022 04:07
@dbeatty10 dbeatty10 requested review from a team as code owners September 16, 2022 04:07
@jtcohen6
Copy link
Contributor

@dbeatty10 I just left a big comment over in #5717 (comment)! I forgot the main PR was over here. What's the difference between these two?

After checking through the codebases, I believe we're in ok shape wrt dbt-server + dbt-cloud

@dbeatty10
Copy link
Contributor Author

If #5717 looks good to you, let's go with that one!

What's the difference between these two?

These two pull requests were different originally, but now I've aligned them.

The original differences were basically these two commits:

They aren't needed due to our discussion here (those commits were only needed if we wanted to look for profiles.yml in the "current working directory of dbt_project.yml").

@jtcohen6
Copy link
Contributor

@dbeatty10 Huzzah! I am thrilled to move forward with whichever of these PRs you wish :)

I think the move now is to close one, and request review on the other. Let's just to make sure someone from the Language or Execution teams has given the code a ✅ before merging.

I do think this change will warrant an update to the docs. Would you mind opening an issue for that, and adding the dbt-core v1.3 label?

@dbeatty10
Copy link
Contributor Author

Closing in favor of #5717

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:yes ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-777] Default to current working directory for profiles.yml
4 participants