chore: Add project command output struct

[lukemassa]

what

Primary change

Break some of fields of ProjectResult into another struct ProjectCommandOutput, which is then embedded into ProjectResult

Secondary changes

1. Plumb SubCommand into command.ProjectContext
2. Add project information to expected output of TestApplyCommandRunner_ExecutionOrder test
3. Add mock to TestPlanCommandRunner_IsSilenced to handle call to Plan

why

Primary change

My main goal here was to clean up the half dozen calls in project_command_runner.go that correspond (roughly) to each possible "project command" from:

func (p *DefaultProjectCommandRunner) Apply(ctx command.ProjectContext) command.ProjectResult {
	applyOut, failure, err := p.doApply(ctx)
	return command.ProjectResult{
		Command:           command.Apply,
		Failure:           failure,
		Error:             err,
		ApplySuccess:      applyOut,
		RepoRelDir:        ctx.RepoRelDir,
		Workspace:         ctx.Workspace,
		ProjectName:       ctx.ProjectName,
		SilencePRComments: ctx.SilencePRComments,
	}
}

to look like:

// Apply runs terraform apply for the project described by ctx.
func (p *DefaultProjectCommandRunner) Apply(ctx command.ProjectContext) command.ProjectCommandOutput {
    applyOut, failure, err := p.doApply(ctx)
    return command.ProjectCommandOutput{
        Failure:      failure,
        Error:        err,  
        ApplySuccess: applyOut,
    }   
}   

The goal here, as described in #5962, was to not have to set the Command as 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 in project_result.go, and consolidating the call site in project_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.

1. We were previously not including SubCommand in ProjectContext, even though we know it in all the same places we know CommandName. This was needed to deal with the hard-coded literal of "rm" that was again essentially "guessed" in StateRm. It was mostly just an exercising in passing an additional argument to functions
2. Now that we don't "lose" the information about "project" in the mocked version of these commands, the output is slightly different. Specifically, when we know the project we add it, whereas when the project is an empty string we omit it, so I needed to add to the expected output strings (again, in a preferable way, since previously we were specifying project in the input but having it missing in the expected output). Example: https://github.com/runatlantis/atlantis/pull/5964/files#diff-bd810f7a2c49ffcc9b60b2cd559bad49dc77687760e0b9073a9c88a362e6985aL357
3. This was a very subtle bug, basically after I made the change the was a unit test that was panicking trying to add to the database. The issue turned out to be that, because the mocked functions "forgot" about command name, the command name was specified as the default (command.Apply), which does not exercise the code path that had the missing pointer. Now that our code keeps its command even after calling mocked version of Plan(), we need to make sure we specify a non-nil value in the PlanSuccess https://github.com/runatlantis/atlantis/pull/5964/files#diff-0afeda625c8feb719ac324deab2c577861662762aecde074307d5a19aa0b1126R129

tests

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