Migrate from dagster-shell to PipesSubprocessClient#57
Migrate from dagster-shell to PipesSubprocessClient#57menzenski wants to merge 8 commits intoquantile-development:masterfrom
Conversation
| full_command = f"{self.meltano_bin} {command}" | ||
| merged_env = {**self.default_env, **env} | ||
|
|
||
| context.log.info(f"Executing command with PipesSubprocessClient: {full_command}") | ||
|
|
||
| client: PipesSubprocessClient = PipesSubprocessClient( | ||
| env=merged_env, | ||
| cwd=self.project_dir, | ||
| context_injector=None, | ||
| ) | ||
| _command: list[str] = full_command.split(" ") | ||
| client.run( | ||
| command=_command, | ||
| context=context, | ||
| ) |
There was a problem hiding this comment.
Using PipesSubprocessClient adds the ability to inject context and get structured messages back from the subprocess. This is useful when the subprocess are python scripts or similar that could be updated to take advantage.
In this case it looks like we are explicitly disabling context injection always invoking a meltano command that I assume can not benefit from these capabilities.
I think the move here might be to just move to a stock subprocess.run call
There was a problem hiding this comment.
I think the move here might be to just move to a stock subprocess.run call
@alangenfeld thanks for that idea - I've opened #58 which does this (it reimplements what dagster-shell had been doing with subprocess).
|
Closing this PR - the approach in #58 is better |
4 participants
For #56
I haven't really started to test this yet but wanted to get something out there for review.