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

[CT-767] [Feature] Supporting on-the-fly configuration of target-path #5399

Closed
1 task done
isidentical opened this issue Jun 21, 2022 · 4 comments · Fixed by #5402
Closed
1 task done

[CT-767] [Feature] Supporting on-the-fly configuration of target-path #5399

isidentical opened this issue Jun 21, 2022 · 4 comments · Fixed by #5402
Labels
enhancement New feature or request help_wanted Trickier changes, with a clear starting point, good for previous/experienced contributors

Comments

@isidentical
Copy link
Contributor

isidentical commented Jun 21, 2022

Is this your first time opening an issue?

Describe the Feature

A flag (like --target-path= or --target-dir=) to customize the target-path variable used by dbt run (and others) temporarily. If there is already a target-path set in the configuration file, this option will override it for the same run.

Describe alternatives you've considered

An environment variable (like DBT_TARGET_PATH) would also be an alternative. The benefits include the ease of implementation (not needing to add the precise config option to each subcommand that needs to deal with target-path). But it would reduce the chances of discovery (since --help won't show it by default).

Who will this benefit?

Anyone who has more than 1 DBT subprocess running at any given time. Our use case involves spawning multiple DBT subprocesses, and sometimes they clash and override each other's run_results.json files.

Are you interested in contributing this feature?

Yes

Anything else?

For more context, please see this thread in DBT's slack channel.

@isidentical isidentical added enhancement New feature or request triage labels Jun 21, 2022
@github-actions github-actions bot changed the title [Feature] Supporting on-the-fly configuration of target-path [CT-767] [Feature] Supporting on-the-fly configuration of target-path Jun 21, 2022
isidentical added a commit to isidentical/dbt-core that referenced this issue Jun 22, 2022
This PR implements `--target-path` CLI flag to customize `target-path` on
a run-level (instead of the existing way of parametrizing it on the project-level).

Resolves dbt-labs#5399.
isidentical added a commit to isidentical/dbt-core that referenced this issue Jun 22, 2022
This PR implements `--target-path` CLI flag to customize `target-path` on
a run-level (instead of the existing way of parametrizing it on the project-level).

Resolves dbt-labs#5399.
@jtcohen6
Copy link
Contributor

@isidentical Thanks for opening!

This feels related to #5132, in that there's a larger desire to override "path" configs set in dbt_project.yml. Similar to "global" configs, I'd envision the order being:

  1. CLI flag (--target-path)
  2. env var (DBT_TARGET_PATH)
  3. Config in version-controlled file (dbt_project.yml)

It's already possible to achieve the precedence in 2+3 by explicitly passing an environment variable into dbt_project.yml:

# dbt_project.yml
target-path: "{{ env_var('TARGET_PATH', 'target') }}"
$ TARGET_PATH=whatever dbt run
$ ls -1 whatever
compiled
graph.gpickle
manifest.json
partial_parse.msgpack
run
run_results.json

I think that sort of env var override should be the default/implicit behavior, too.

So, I'm not strictly opposed to the one-off addition in #5402. But what I'd really love is a consistent approach that:

  • achieves this consistently for all "runtime" path configs (at least both log-path + target-path)
  • explicitly enables setting these via CLI flag or DBT_-prefixed env var, following logic in the flags module

Would you be interested in attempting that in #5402?

@jtcohen6 jtcohen6 added help_wanted Trickier changes, with a clear starting point, good for previous/experienced contributors Team:Language and removed triage labels Jun 28, 2022
@jtcohen6
Copy link
Contributor

Also: We should find some way to square config that's set in dbt_project.yml versus profiles.yml, in cases where it makes sense to unify both under "global" configs. In the past, config that's relevant to project directory files tended to go in the project file, but there's also a compelling proposal to support profiles.yml in the same directory by default: #5411

That's out of the scope of this issue/PR, but it felt like a relevant place to leave the thought for now.

@isidentical
Copy link
Contributor Author

Thanks a lot for your answer @jtcohen6! Two quick questions before I start the implementation:

I think that sort of env var override should be the default/implicit behavior, too.

explicitly enables setting these via CLI flag or DBT_-prefixed env var, following logic in the flags module

Should #5402 initially aim for adding only the environment variables (which would solve the issue for us / and for the other people who have the use cases I've shared) or should it add both the env vars and the cli flags?

For the environment variables, are the following namings make sense or should we consider DBT_CONFIG_ prefix?

  • DBT_TARGET_PATH
  • DBT_LOG_PATH

@jtcohen6
Copy link
Contributor

jtcohen6 commented Jun 28, 2022

I'd much prefer to thread these through the same centralized flags logic linked above, rather than one-off calls to os.getenv nested deep in the dbt_project.yml parsing logic. At that point, it's probably not much additional work to support both CLI flags and env vars.

The DBT_ prefix feels appropriate, and consistent with all other global configs.

isidentical added a commit to isidentical/dbt-core that referenced this issue Jun 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help_wanted Trickier changes, with a clear starting point, good for previous/experienced contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants