Skip to content

Commit

Permalink
fix(service): don't change working directory, as it isn't thread-safe (
Browse files Browse the repository at this point in the history
  • Loading branch information
Panaetius committed Oct 26, 2022
1 parent b81bbe5 commit 909b001
Show file tree
Hide file tree
Showing 4 changed files with 40 additions and 12 deletions.
10 changes: 7 additions & 3 deletions renku/command/command_builder/command.py
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ def __init__(self) -> None:
self._finalized: bool = False
self._track_std_streams: bool = False
self._working_directory: Optional[str] = None
self._context_added: bool = False

def __getattr__(self, name: str) -> Any:
"""Bubble up attributes of wrapped builders."""
Expand All @@ -165,8 +166,10 @@ def _injection_pre_hook(self, builder: "Command", context: dict, *args, **kwargs
builder("Command"): Current ``CommandBuilder``.
context(dict): Current context dictionary.
"""
path = get_git_path(self._working_directory or ".")
project_context.push_path(path)
if not project_context.has_context():
path = get_git_path(self._working_directory or ".")
project_context.push_path(path)
self._context_added = True

context["bindings"] = {}
context["constructor_bindings"] = {}
Expand All @@ -192,7 +195,8 @@ def _post_hook(self, builder: "Command", context: dict, result: "CommandResult",
"""
remove_injector()

project_context.pop_context()
if self._context_added:
project_context.pop_context()

if result.error:
raise result.error
Expand Down
2 changes: 1 addition & 1 deletion renku/core/util/contexts.py
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ def renku_project_context(path, check_git_path=True):
if check_git_path:
path = get_git_path(path)

with project_context.with_path(path=path), chdir(path):
with project_context.with_path(path=path):
project_context.external_storage_requested = True
yield project_context.path

Expand Down
5 changes: 0 additions & 5 deletions renku/domain_model/project_context.py
Original file line number Diff line number Diff line change
Expand Up @@ -99,11 +99,6 @@ def docker_path(self):
"""Path to the Dockerfile."""
return self.path / DOCKERFILE

@property
def empty(self) -> bool:
"""Return True if there is no pushed context."""
return not bool(self._context_stack)

@property
def global_config_dir(self) -> str:
"""Return user's config directory."""
Expand Down
35 changes: 32 additions & 3 deletions tests/core/test_project_context.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
from renku.domain_model.project_context import ProjectContext


def _use_project_context(tmpdir):
def test_use_project_context(tmpdir):
"""Test ProjectContext object."""
project_context = ProjectContext()

Expand Down Expand Up @@ -100,7 +100,7 @@ def test_database(tmpdir):
_ = project_context.database


def _use_project_context_with_path():
def test_use_project_context_with_path():
"""Test ProjectContext.with_path leaves the state as before."""
project_context = ProjectContext()

Expand All @@ -115,7 +115,7 @@ def _use_project_context_with_path():
assert before_path == project_context.path


def _use_project_context_with_path_empty():
def test_use_project_context_with_path_empty():
"""Test ProjectContext.with_path leaves the state as before when no path was pushed before."""
project_context = ProjectContext()

Expand All @@ -134,3 +134,32 @@ def test_get_repository_outside_a_project(tmpdir):
with project_context.with_path(tmpdir.mkdir("project")):
with pytest.raises(errors.ConfigurationError):
_ = project_context.repository


def test_reuse_context(tmp_path):
"""Test that the same context can be used for multiple commands."""
from renku.command.command_builder.command import Command
from renku.domain_model.project_context import project_context

def _test():
"""Dummy method for testing."""
pass

testdir = tmp_path / "test"
testdir.mkdir(parents=True, exist_ok=True)

assert not project_context.has_context()

with project_context.with_path(testdir):
assert project_context.has_context()

Command().command(_test).build().execute()
assert project_context.has_context()

Command().command(_test).build().execute()
assert project_context.has_context()

Command().command(_test).build().execute()
assert project_context.has_context()

assert not project_context.has_context()

0 comments on commit 909b001

Please sign in to comment.