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

Consolidate *Cmd logic #120

Closed
azriel91 opened this issue May 3, 2023 · 1 comment · Fixed by #130
Closed

Consolidate *Cmd logic #120

azriel91 opened this issue May 3, 2023 · 1 comment · Fixed by #130

Comments

@azriel91
Copy link
Owner

azriel91 commented May 3, 2023

Currently:

  • EnsureCmd and CleanCmd delegate to ApplyCmd.
  • ApplyCmd and StatesDiscoverCmd both collect outcomes to surface errors as a whole.

What we would like for all commands:

  • Ability to use other commands' logic, but all of the *Cmd invocations share one progress output.
  • Output type is generic
  • Collect errors from each item spec, and return all values and errors as a whole.

Use cases:

  • EnsureCmd:

    • Forward iteration through each item.

      Calls state_current, apply_check, apply_exec in sequence.

  • CleanCmd

    • Forward iteration through each item.

      Calls try_state_current.

    • Reverse iteration through each item.

      Calls apply_check, apply_exec in sequence.

  • StatesDiscoverCmd:

    • Forward iteration through each item.

      Calls try_state_current / try_state_desired.

@azriel91
Copy link
Owner Author

azriel91 commented Jun 5, 2023

Design / implementation issue:

  1. ApplyCmd wants to call StatesDiscoverCmd::current
  2. Inlining the state current discovery logic into ApplyCmd, and executing concurrently requires an intemediate outcomes rx, which is essentially re-implementing CmdBase.
  3. Current logic of StatesDiscoverCmd takes in &mut CmdCtx.
  4. For automation logic, StatesDiscoverCmd needs immutable references to the flow and parameter specs.
  5. However, CmdCtx and SingleProfileSingleFlowView contain &mut Output, and &mut CmdProgressTracker.
  6. This makes it hard to pass the container of flow / parameter information around.
  7. Only the top level command needs &mut Output and &mut CmdProgressTracker to set up progress rendering.
  8. We could move those two out of the view and into the CmdCtx, so that fn_exec can take in the *View and progress and outcome Senders, and this view can be passed to sub commands.
  9. If we don't do that, then every command needs to pass through about 5 separate parameters.

So, we should split Output and CmdProgressTracker out of the view into ScopeViewAndOutput.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant