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

Adds chdir before and after dbt run #210

Merged
merged 7 commits into from
Apr 3, 2023

Conversation

racheldaniel
Copy link
Contributor

@racheldaniel racheldaniel commented Mar 31, 2023

What is this PR?

This is a:

  • documentation update
  • bug fix with no breaking changes
  • new functionality
  • a breaking change

All pull requests from community contributors should target the main branch (default).

Description & motivation

Until core has completed this bug ticket, we need to intentionally chdir to the project root before running a dbt command, then chdir back to the server root afterward. This compensates for core 1. Writing an empty dbt_packages folder to wherever the command is being called (in our case, the server root, which we do not want)
2. Changing directories to the project root for a deps and not changing back, which prevents the server from specifying a task artifacts/project state folder that is relative to the server code path.

Checklist

  • I have verified that these changes work locally on the following warehouses (Note: it's okay if you do not have access to all warehouses, this helps us understand what has been covered)
    • BigQuery
    • Postgres
    • Redshift
    • Snowflake
    • Databricks
    • Spark
  • I have updated the README.md (if applicable)
  • I have added tests & descriptions to my models
  • I have added an entry to CHANGELOG.md

@cla-bot cla-bot bot added the cla:yes label Mar 31, 2023
@github-actions
Copy link

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.

@@ -103,9 +128,53 @@ def _insert_log_path(command: List[str], task_id: str):
return
command.insert(0, LOG_PATH_ARGS)
command.insert(1, get_task_artifacts_path(task_id, None))
command.insert(2, LOG_FORMAT_ARGS)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm just curious do you prefer let IDE set in request explicitly? Or have a hard coded code here.

I tends to not make any unnecessary hard coded logic in worker because it's implicit and hard to debug. Because log format is relatively trivial so I'm neutral on this. Let me know you final decision.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we'd prefer to set it here. Dbt CLI defaults to text format, and it doesn't make a lot of sense for dbt-server to use that default if we're also setting a --log-path for a default (the text format isn't easily read from a log file). We also don't want the IDE to have to specify the format on every request. I think if in the end the dbt-server is actually used for multiple cases, we could revisit and possibly set with an env var, but for the time being I think this is a sensible default.

@racheldaniel racheldaniel merged commit 9e41f67 into main Apr 3, 2023
@racheldaniel racheldaniel deleted the racheld/BUG/chdir-to-avoid-stray-artifacts branch April 3, 2023 12:44
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.

2 participants