-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Description
Community Note
- Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request. Searching for pre-existing feature requests helps us consolidate datapoints for identical requirements into a single place, thank you!
- Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request.
- If you are interested in working on this issue or have submitted a pull request, please leave a comment.
- I'd be willing to implement this feature (contributing guide)
Describe the user story
Right now for each command there is a block like this:
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,
}
}
The runner is "inventing" the name of the command that it assumed called it. Higher up in the call stack the command is known, then we "forget it" and sneak it back in here. This causes bugs like #5934, and in general doesn't make a lot of sense. In addition the ctx.* content (which again is returned by all the analogous commands) is a code smell here; ctx is being passed in to this function, the caller clearly knows things like ProjectName or Workspace, why are we telling it?
The issue is that the type ProjectResult is used in many places, and here is doing double duty of summarizing what happened in a given run, as well as the output from a given command.
Describe the solution you'd like
These functions should return a pared down ProjectCommandOutput, that the caller of this function should then "decorate" with the additional information.
Describe the drawbacks of your solution
It's a refactor so there's some risk there, but there's a lot of test coverage so should be ok.
Describe alternatives you've considered
I can't think of any