-
Notifications
You must be signed in to change notification settings - Fork 0
Address review feedback for state attribute feature #1098
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
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -101,6 +101,7 @@ async def run_stages( | |
|
|
||
| class Gremlin: | ||
| registry: ArtifactRegistry | ||
| state: State | None | ||
|
|
||
| def __init__( | ||
| self, | ||
|
|
@@ -150,6 +151,7 @@ def __init__( | |
| self.fetch_worktree = fetch_worktree | ||
| self.pipeline_path = pipeline_path | ||
| self.pipeline_args = pipeline_args or [] | ||
| self.state = None | ||
|
|
||
| @property | ||
| def artifact_dir(self) -> pathlib.Path: | ||
|
|
@@ -276,14 +278,17 @@ def _set_gremlin_recursive(self, stage: StageProtocol) -> None: | |
| for nested in body: | ||
| self._set_gremlin_recursive(nested) | ||
|
|
||
| def _collect_stages( | ||
| self, stages: Sequence[StageProtocol] | ||
| ) -> list[tuple[str, Callable[[], Awaitable[Any]]]]: | ||
| cwd = ( | ||
| def _resolve_cwd(self) -> str: | ||
| return ( | ||
| str(self.worktree_dir) | ||
| if self.worktree_dir is not None | ||
| else (self.project_root or str(pathlib.Path.cwd())) | ||
| ) | ||
|
|
||
| def _collect_stages( | ||
| self, stages: Sequence[StageProtocol] | ||
| ) -> list[tuple[str, Callable[[], Awaitable[Any]]]]: | ||
| cwd = self._resolve_cwd() | ||
| built: list[tuple[str, Callable[[], Awaitable[Any]]]] = [] | ||
| for e in stages: | ||
| self._set_gremlin_recursive(e) | ||
|
|
@@ -300,7 +305,16 @@ def _collect_stages( | |
| artifacts=self.registry, | ||
| base_ref=self.base_ref, | ||
| ) | ||
| built.append((e.name, stage_state.make_runner(e, scope=stages))) | ||
| base_runner = stage_state.make_runner(e, scope=stages) | ||
|
|
||
| async def _set_state_and_run( | ||
| runner: Callable[[], Awaitable[Any]] = base_runner, | ||
| state: State = stage_state, | ||
| ) -> Any: | ||
| self.state = state | ||
| return await runner() | ||
|
|
||
| built.append((e.name, _set_state_and_run)) | ||
|
Comment on lines
+308
to
+317
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed. Wrapped nested stage runners in sequence, loop, and parallel stages to set child.gremlin.state before execution. This ensures nested stages have access to their correct child state. |
||
| return built | ||
|
|
||
| def _unbind_stale_exec_artifacts(self) -> None: | ||
|
|
@@ -512,6 +526,20 @@ def initialize_with_runtime( | |
| sha = _git_mod.head_sha(cwd=self.worktree_dir) | ||
| if sha: | ||
| self.registry.bind("base_sha", Uri.parse(f"git://commit/{sha}")) | ||
|
|
||
| cwd = self._resolve_cwd() | ||
| self.state = build_state( | ||
| data=StateData(gremlin_id=self.gremlin_id, state_file=self.state_file), | ||
| client=resolved_client or PACKAGE_DEFAULT, | ||
| artifact_dir=self.artifact_dir, | ||
| pipeline_data=self.pipeline_data, | ||
| repo=self.repo, | ||
| cwd=cwd, | ||
| worktree=self.worktree_dir, | ||
| worktree_parent=self.worktree_parent, | ||
| artifacts=self.registry, | ||
| base_ref=self.base_ref, | ||
| ) | ||
| except Exception: | ||
| if worktree_created: | ||
| _git_mod.remove_worktree(self.project_root, worktree_created) | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,145 @@ | ||
| """Tests for Gremlin.state attribute.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import asyncio | ||
| import subprocess | ||
|
|
||
| import pytest | ||
|
|
||
| from gremlins.executor.gremlin import Gremlin | ||
| from gremlins.executor.state import State | ||
| from gremlins.protocols import StageProtocol | ||
| from gremlins.stages.outcome import Done | ||
|
|
||
|
|
||
| def _init_git_repo(path) -> None: | ||
| subprocess.run(["git", "init"], cwd=str(path), check=True, capture_output=True) | ||
| subprocess.run( | ||
| ["git", "config", "user.name", "Test"], | ||
| cwd=str(path), | ||
| check=True, | ||
| capture_output=True, | ||
| ) | ||
| subprocess.run( | ||
| ["git", "config", "user.email", "test@test.com"], | ||
| cwd=str(path), | ||
| check=True, | ||
| capture_output=True, | ||
| ) | ||
| subprocess.run( | ||
| ["git", "add", "-A"], | ||
| cwd=str(path), | ||
| check=True, | ||
| capture_output=True, | ||
| ) | ||
| subprocess.run( | ||
| ["git", "commit", "-m", "init"], | ||
| cwd=str(path), | ||
| check=True, | ||
| capture_output=True, | ||
| ) | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def project_dir(tmp_path): | ||
| """Git repository for testing.""" | ||
| d = tmp_path / "project" | ||
| d.mkdir() | ||
| (d / "file.txt").write_text("hello") | ||
| _init_git_repo(d) | ||
| return d | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def pipeline_yaml(tmp_path): | ||
| p = tmp_path / "trivial.yaml" | ||
| p.write_text( | ||
| """\ | ||
| stages: | ||
| - name: test | ||
| type: exec | ||
| options: | ||
| cmds: | ||
| - "true" | ||
| """ | ||
| ) | ||
| return p | ||
|
|
||
|
|
||
| def test_gremlin_state_populated_after_initialize(project_dir, pipeline_yaml, sandbox): | ||
| """Gremlin.state is non-None after initialize_with_runtime.""" | ||
| gremlin_id = "test-state-init" | ||
| state_dir = sandbox.state / gremlin_id | ||
|
|
||
| gremlin = Gremlin.initialize_with_runtime( | ||
| gremlin_id=gremlin_id, | ||
| state_dir=state_dir, | ||
| project_dir=project_dir, | ||
| pipeline_ref=str(pipeline_yaml), | ||
| project_root=str(project_dir), | ||
| ) | ||
|
|
||
| assert gremlin.state is not None | ||
|
|
||
|
|
||
| def test_gremlin_state_attributes_accessible(project_dir, pipeline_yaml, sandbox): | ||
| """Gremlin.state attributes are accessible after initialize_with_runtime.""" | ||
| gremlin_id = "test-state-attrs" | ||
| state_dir = sandbox.state / gremlin_id | ||
|
|
||
| gremlin = Gremlin.initialize_with_runtime( | ||
| gremlin_id=gremlin_id, | ||
| state_dir=state_dir, | ||
| project_dir=project_dir, | ||
| pipeline_ref=str(pipeline_yaml), | ||
| project_root=str(project_dir), | ||
| ) | ||
|
|
||
| assert gremlin.state is not None | ||
| assert gremlin.state.client is not None | ||
| assert gremlin.state.artifacts is not None | ||
| assert gremlin.state.artifact_dir is not None | ||
|
|
||
|
|
||
| class StateCapturingStage(StageProtocol): | ||
| """Test stage that captures gremlin.state when run.""" | ||
|
|
||
| def __init__(self): | ||
| self.name = "capture" | ||
| self.type = "test" | ||
| self.path = "capture" | ||
| self.gremlin = None | ||
| self.client = None | ||
| self.captured_state = None | ||
| self.skip_if_exists = None | ||
|
|
||
| async def run(self, state: State): | ||
| """Capture the gremlin.state value during execution.""" | ||
| if self.gremlin: | ||
| self.captured_state = self.gremlin.state | ||
| return Done() | ||
|
|
||
|
|
||
| def test_gremlin_state_set_before_stage_run(project_dir, pipeline_yaml, sandbox): | ||
| """Gremlin.state is set before stage.run() is called.""" | ||
| gremlin_id = "test-state-runner" | ||
| state_dir = sandbox.state / gremlin_id | ||
|
|
||
| gremlin = Gremlin.initialize_with_runtime( | ||
| gremlin_id=gremlin_id, | ||
| state_dir=state_dir, | ||
| project_dir=project_dir, | ||
| pipeline_ref=str(pipeline_yaml), | ||
| project_root=str(project_dir), | ||
| ) | ||
|
|
||
| stage = StateCapturingStage() | ||
|
|
||
| collected = gremlin._collect_stages([stage]) | ||
| assert len(collected) > 0 | ||
| assert collected[0][0] == "capture" | ||
|
|
||
| asyncio.run(collected[0][1]()) | ||
| assert stage.captured_state is not None | ||
| assert stage.captured_state == gremlin.state |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added state: Any to GremlinProtocol to enable type checking for stage code using self.gremlin.state.