feat(builder): Introduce templating for prereq commands #187
Conversation
There was a problem hiding this comment.
This looks great, and tests look handy too! As for the PR title, the scopes typically relate to the subparts of that particular project, usually the folder it's in. In this case, it'd be feat(builder). We don't yet have any of the automation set up that ties any of it together, but that'll be coming soon!
builder/builder.go
Outdated
| outputLog, err := util.RunInDir(p.Command, runnable.Fullpath) | ||
| fullCmd, err := p.GetCommand(runnable) | ||
| if err != nil { | ||
| return errors.Wrap(err, "failed to get templated full command") |
There was a problem hiding this comment.
It maybe seems a little silly, but the convention we use for error messages is failed to + the name of the function that failed. Something like this:
| return errors.Wrap(err, "failed to get templated full command") | |
| return errors.Wrap(err, "failed to GetCommand for preReq") |
There was a problem hiding this comment.
I'll change that up. Also somewhat relatedly, what do you think about using this? https://thanos.io/tip/contributing/coding-style-guide.md/#wrap-errors-for-more-context-dont-repeat-failed--there
NOTE: never prefix wrap messages with wording like failed ... or error occurred while.... Just describe what we wanted to do when the failure occurred. Those prefixes are just noise. We are wrapping error, so it’s obvious that some error occurred, right? (: Improve readability and consider avoiding those.
There was a problem hiding this comment.
I changed up the errors in 64cb16d.
Additionally I see that result.OutputLog gets the log from util.RunInDir but it's not being used anywhere that I can see. A global search doesn't reveal anything where OutputLog gets passed to a log function call, or even to an fmt.
There was a problem hiding this comment.
@javorszky I'm a fan of that honestly. Will ping @cohix for an opinion as well since he came up with the format that's used currently.
javorszky
changed the title
|
Hm, interesting that a new commit does not dismiss the approval. 🤔 |
Closes #123
Adds templating to prerequisite commands
Prereqstruct and out of the builder