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

Target path should be relative to project dir, rather than current working directory #7706

Merged
merged 13 commits into from
May 26, 2023
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changes/unreleased/Fixes-20230525-165053.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
kind: Fixes
body: Incorrect paths used for "target" and "state" directories
time: 2023-05-25T16:50:53.718564-04:00
custom:
Author: gshank
Issue: "7465"
2 changes: 1 addition & 1 deletion core/dbt/cli/params.py
Original file line number Diff line number Diff line change
Expand Up @@ -431,7 +431,7 @@
dir_okay=True,
file_okay=False,
readable=True,
resolve_path=True,
resolve_path=False,
path_type=Path,
),
)
Expand Down
2 changes: 1 addition & 1 deletion core/dbt/cli/requires.py
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ def wrapper(*args, **kwargs):

ctx.obj["manifest"] = manifest
if write and ctx.obj["flags"].write_json:
write_manifest(manifest, ctx.obj["runtime_config"].target_path)
write_manifest(manifest, ctx.obj["runtime_config"].project_target_path)

return func(*args, **kwargs)

Expand Down
13 changes: 7 additions & 6 deletions core/dbt/compilation.py
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ def __init__(self, config):
self.config = config

def initialize(self):
make_directory(self.config.target_path)
make_directory(self.config.project_target_path)
make_directory(self.config.packages_install_path)

# creates a ModelContext which is converted to
Expand Down Expand Up @@ -512,7 +512,9 @@ def compile(self, manifest: Manifest, write=True, add_test_edges=False) -> Graph
# including the test edges.
summaries["with_test_edges"] = linker.get_graph_summary(manifest)

with open(os.path.join(self.config.target_path, "graph_summary.json"), "w") as out_stream:
with open(
os.path.join(self.config.project_target_path, "graph_summary.json"), "w"
) as out_stream:
try:
out_stream.write(json.dumps(summaries))
except Exception as e: # This is non-essential information, so merely note failures.
Expand All @@ -539,7 +541,7 @@ def compile(self, manifest: Manifest, write=True, add_test_edges=False) -> Graph

def write_graph_file(self, linker: Linker, manifest: Manifest):
filename = graph_file_name
graph_path = os.path.join(self.config.target_path, filename)
graph_path = os.path.join(self.config.project_target_path, filename)
flags = get_flags()
if flags.WRITE_JSON:
linker.write_graph(graph_path, manifest)
Expand All @@ -554,9 +556,8 @@ def _write_node(self, node: ManifestSQLNode) -> ManifestSQLNode:
fire_event(WritingInjectedSQLForNode(node_info=get_node_info()))

if node.compiled_code:
node.compiled_path = node.write_node(
self.config.target_path, "compiled", node.compiled_code
)
node.compiled_path = node.get_target_write_path(self.config.target_path, "compiled")
node.write_node(self.config.project_root, node.compiled_path, node.compiled_code)
return node

def compile_node(
Expand Down
5 changes: 5 additions & 0 deletions core/dbt/config/project.py
Original file line number Diff line number Diff line change
Expand Up @@ -700,3 +700,8 @@ def get_macro_search_order(self, macro_namespace: str):
if dispatch_entry["macro_namespace"] == macro_namespace:
return dispatch_entry["search_order"]
return None

@property
def project_target_path(self):
# If target_path is absolute, project_root will not be included
return os.path.join(self.project_root, self.target_path)
3 changes: 2 additions & 1 deletion core/dbt/context/providers.py
Original file line number Diff line number Diff line change
Expand Up @@ -833,7 +833,8 @@ def write(self, payload: str) -> str:
# macros/source defs aren't 'writeable'.
if isinstance(self.model, (Macro, SourceDefinition)):
raise MacrosSourcesUnWriteableError(node=self.model)
self.model.build_path = self.model.write_node(self.config.target_path, "run", payload)
self.model.build_path = self.model.get_target_write_path(self.config.target_path, "run")
self.model.write_node(self.config.project_root, self.model.build_path, payload)
return ""

@contextmember
Expand Down
14 changes: 10 additions & 4 deletions core/dbt/contracts/graph/nodes.py
Original file line number Diff line number Diff line change
Expand Up @@ -345,17 +345,23 @@ class ParsedNode(NodeInfoMixin, ParsedNodeMandatory, SerializableType):
relation_name: Optional[str] = None
raw_code: str = ""

def write_node(self, target_path: str, subdirectory: str, payload: str):
def get_target_write_path(self, target_path: str, subdirectory: str):
Copy link
Member

Choose a reason for hiding this comment

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

Can we convert this to a custom context manager, so we can centralize logic in one place and use with statements?

https://realpython.com/python-with-statement/#creating-custom-context-managers

# This is called for both the "compiled" subdirectory of "target" and the "run" subdirectory
if os.path.basename(self.path) == os.path.basename(self.original_file_path):
# One-to-one relationship of nodes to files.
path = self.original_file_path
else:
# Many-to-one relationship of nodes to files.
path = os.path.join(self.original_file_path, self.path)
full_path = os.path.join(target_path, subdirectory, self.package_name, path)
target_write_path = os.path.join(target_path, subdirectory, self.package_name, path)
return target_write_path

write_file(full_path, payload)
return full_path
def write_node(self, project_root: str, compiled_path, compiled_code: str):
if os.path.isabs(compiled_path):
full_path = compiled_path
else:
full_path = os.path.join(project_root, compiled_path)
write_file(full_path, compiled_code)

def _serialize(self):
return self.to_dict()
Expand Down
16 changes: 9 additions & 7 deletions core/dbt/contracts/state.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,31 +7,33 @@


class PreviousState:
def __init__(self, path: Path, current_path: Path):
self.path: Path = path
self.current_path: Path = current_path
def __init__(self, state_path: Path, target_path: Path, project_root: Path):
self.state_path: Path = state_path
self.target_path: Path = target_path
self.project_root: Path = project_root
self.manifest: Optional[WritableManifest] = None
self.results: Optional[RunResultsArtifact] = None
self.sources: Optional[FreshnessExecutionResultArtifact] = None
self.sources_current: Optional[FreshnessExecutionResultArtifact] = None

manifest_path = self.path / "manifest.json"
# Note: if state_path is absolute, project_root will be ignored.
manifest_path = self.project_root / self.state_path / "manifest.json"
if manifest_path.exists() and manifest_path.is_file():
try:
self.manifest = WritableManifest.read_and_check_versions(str(manifest_path))
except IncompatibleSchemaError as exc:
exc.add_filename(str(manifest_path))
raise

results_path = self.path / "run_results.json"
results_path = self.project_root / self.state_path / "run_results.json"
if results_path.exists() and results_path.is_file():
try:
self.results = RunResultsArtifact.read_and_check_versions(str(results_path))
except IncompatibleSchemaError as exc:
exc.add_filename(str(results_path))
raise

sources_path = self.path / "sources.json"
sources_path = self.project_root / self.state_path / "sources.json"
if sources_path.exists() and sources_path.is_file():
try:
self.sources = FreshnessExecutionResultArtifact.read_and_check_versions(
Expand All @@ -41,7 +43,7 @@ def __init__(self, path: Path, current_path: Path):
exc.add_filename(str(sources_path))
raise

sources_current_path = self.current_path / "sources.json"
sources_current_path = self.target_path / "sources.json"
MichelleArk marked this conversation as resolved.
Show resolved Hide resolved
if sources_current_path.exists() and sources_current_path.is_file():
try:
self.sources_current = FreshnessExecutionResultArtifact.read_and_check_versions(
Expand Down
10 changes: 3 additions & 7 deletions core/dbt/parser/manifest.py
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ def get_full_manifest(
loader.track_project_load()

if write_perf_info:
loader.write_perf_info(config.target_path)
loader.write_perf_info(config.project_target_path)

return manifest

Expand Down Expand Up @@ -729,9 +729,7 @@ def macro_depends_on(self):
macro.depends_on.add_macro(dep_macro_id) # will check for dupes

def write_manifest_for_partial_parse(self):
path = os.path.join(
self.root_project.project_root, self.root_project.target_path, PARTIAL_PARSE_FILE_NAME
)
path = os.path.join(self.root_project.project_target_path, PARTIAL_PARSE_FILE_NAME)
try:
# This shouldn't be necessary, but we have gotten bug reports (#3757) of the
# saved manifest not matching the code version.
Expand Down Expand Up @@ -944,9 +942,7 @@ def read_manifest_for_partial_parse(self) -> Optional[Manifest]:
if not get_flags().PARTIAL_PARSE:
fire_event(PartialParsingNotEnabled())
return None
path = os.path.join(
self.root_project.project_root, self.root_project.target_path, PARTIAL_PARSE_FILE_NAME
)
path = os.path.join(self.root_project.project_target_path, PARTIAL_PARSE_FILE_NAME)

reparse_reason = None

Expand Down
2 changes: 1 addition & 1 deletion core/dbt/task/compile.py
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ def defer_to_manifest(self, adapter, selected_uids: AbstractSet[str]):
favor_state=bool(self.args.favor_state),
)
# TODO: is it wrong to write the manifest here? I think it's right...
write_manifest(self.manifest, self.config.target_path)
write_manifest(self.manifest, self.config.project_target_path)

def _runtime_initialize(self):
if getattr(self.args, "inline", None):
Expand Down
2 changes: 1 addition & 1 deletion core/dbt/task/freshness.py
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ def result_path(self):
if self.args.output:
return os.path.realpath(self.args.output)
else:
return os.path.join(self.config.target_path, RESULT_FILE_NAME)
return os.path.join(self.config.project_target_path, RESULT_FILE_NAME)

def raise_on_first_error(self):
return False
Expand Down
10 changes: 6 additions & 4 deletions core/dbt/task/generate.py
Original file line number Diff line number Diff line change
Expand Up @@ -214,10 +214,12 @@ def run(self) -> CatalogArtifact:
compile_results=compile_results,
)

shutil.copyfile(DOCS_INDEX_FILE_PATH, os.path.join(self.config.target_path, "index.html"))
shutil.copyfile(
DOCS_INDEX_FILE_PATH, os.path.join(self.config.project_target_path, "index.html")
)

for asset_path in self.config.asset_paths:
to_asset_path = os.path.join(self.config.target_path, asset_path)
to_asset_path = os.path.join(self.config.project_target_path, asset_path)

if os.path.exists(to_asset_path):
shutil.rmtree(to_asset_path)
Expand Down Expand Up @@ -257,10 +259,10 @@ def run(self) -> CatalogArtifact:
errors=errors,
)

path = os.path.join(self.config.target_path, CATALOG_FILENAME)
path = os.path.join(self.config.project_target_path, CATALOG_FILENAME)
results.write(path)
if self.args.compile:
write_manifest(self.manifest, self.config.target_path)
write_manifest(self.manifest, self.config.project_target_path)

if exceptions:
fire_event(WriteCatalogFailure(num_exceptions=len(exceptions)))
Expand Down
2 changes: 1 addition & 1 deletion core/dbt/task/run_operation.py
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ def run(self) -> RunResultsArtifact:
results=[run_result],
)

result_path = os.path.join(self.config.target_path, RESULT_FILE_NAME)
result_path = os.path.join(self.config.project_target_path, RESULT_FILE_NAME)

if self.args.write_json:
results.write(result_path)
Expand Down
8 changes: 5 additions & 3 deletions core/dbt/task/runnable.py
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,9 @@ def __init__(self, args, config, manifest):
def set_previous_state(self):
if self.args.state is not None:
self.previous_state = PreviousState(
path=self.args.state, current_path=Path(self.config.target_path)
state_path=self.args.state,
target_path=Path(self.config.target_path),
project_root=Path(self.config.project_root),
)

def index_offset(self, value: int) -> int:
Expand Down Expand Up @@ -156,7 +158,7 @@ def get_runner_type(self, node):
raise NotImplementedError("Not Implemented")

def result_path(self):
return os.path.join(self.config.target_path, RESULT_FILE_NAME)
return os.path.join(self.config.project_target_path, RESULT_FILE_NAME)

def get_runner(self, node):
adapter = get_adapter(self.config)
Expand Down Expand Up @@ -455,7 +457,7 @@ def run(self):
)

if self.args.write_json:
write_manifest(self.manifest, self.config.target_path)
write_manifest(self.manifest, self.config.project_target_path)
if hasattr(result, "write"):
result.write(self.result_path())

Expand Down
2 changes: 1 addition & 1 deletion core/dbt/task/serve.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@

class ServeTask(ConfiguredTask):
def run(self):
os.chdir(self.config.target_path)
os.chdir(self.config.project_target_path)
shutil.copyfile(DOCS_INDEX_FILE_PATH, "index.html")

port = self.args.port
Expand Down
4 changes: 3 additions & 1 deletion test/unit/test_graph_selector_methods.py
Original file line number Diff line number Diff line change
Expand Up @@ -1202,7 +1202,9 @@ def test_select_metric(manifest):
def previous_state(manifest):
writable = copy.deepcopy(manifest).writable_manifest()
state = PreviousState(
path=Path("/path/does/not/exist"), current_path=Path("/path/does/not/exist")
state_path=Path("/path/does/not/exist"),
target_path=Path("/path/does/not/exist"),
project_root=Path("/path/does/not/exist"),
)
state.manifest = writable
return state
Expand Down
6 changes: 4 additions & 2 deletions tests/functional/configs/test_configs.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,11 +58,13 @@ def project_config_update(self):
}

def test_alternative_target_paths(self, project):
# chdir to a different directory to test creation of target directory under project_root
os.chdir(project.profiles_dir)
run_dbt(["seed"])

target_path = ""
for d in os.listdir("."):
if os.path.isdir(d) and d.startswith("target_"):
for d in os.listdir(project.project_root):
if os.path.isdir(os.path.join(project.project_root, d)) and d.startswith("target_"):
target_path = d
assert os.path.exists(os.path.join(project.project_root, target_path, "manifest.json"))

Expand Down
Loading