chore: Add project command output struct #5964
Open
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.
what
Primary change
Break some of fields of
ProjectResultinto another structProjectCommandOutput, which is then embedded intoProjectResultSecondary changes
SubCommandintocommand.ProjectContextprojectinformation to expected output of TestApplyCommandRunner_ExecutionOrder testTestPlanCommandRunner_IsSilencedto handle call to Planwhy
Primary change
My main goal here was to clean up the half dozen calls in
project_command_runner.gothat correspond (roughly) to each possible "project command" from:to look like:
The goal here, as described in #5962, was to not have to set the
Commandas part what is returned from a the run. This makes conceptual sense; the caller of this function knows it was told to do so by "apply", why is the return of this function including that? It's not purely academic either, as we add commands, the translation might not be so easily 1:1 (for example "autoplan").In addition, we are copy/pasting these returns for RepoRelDir, Workspace, etc., into each command, which is error prone and brittle. The new logic has each command return just what the command returns, then the call site adds in the additional context (that it already knows) before passing it up.
This would have prevented a few very subtle bugs, like #5934 and #5963, since we rely less on specifying the exact right things, in either production code or unit tests.
Secondary changes
Most of the work to make the above work, outside of the intentional changes to
project_command_runner.go, the new types inproject_result.go, and consolidating the call site inproject_command_pool_executor.go, was highly mechanical, just an exercise in breaking up types in unit tests. There were however some additional changes that were necessary to make this work.All of these changes of which ended up being improvements, albeit minor ones. This gave me confidence that this change was the right direction, since it was setting up the code to be cleaner.
"rm"that was again essentially "guessed" inStateRm. It was mostly just an exercising in passing an additional argument to functionstests
This is a refactor so should be safe.
I'm not sure the "command" is used anywhere but logs and outputs anyway
references
closes: #5962
Would have prevented bugs: #5934 and #5963