-
Notifications
You must be signed in to change notification settings - Fork 1k
fix(ensure): concurrent update args validation #1175
fix(ensure): concurrent update args validation #1175
Conversation
cmd/dep/ensure.go
Outdated
| // Log all the errors | ||
| if len(errArgsValidationCh) > 0 { | ||
| for err := range errArgsValidationCh { | ||
| ctx.Err.Println(err.Error()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we print a line to give some context to these errors?
Something like: Invalid arguments passed to ensure -update:
cdfe942 to
9683e43
Compare
|
Made the error output similar to manifest validation output with |
ibrasho
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
cmd/dep/ensure.go
Outdated
|
|
||
| func validateUpdateArgs(ctx *dep.Ctx, args []string, p *dep.Project, sm gps.SourceManager, params *gps.SolveParameters) error { | ||
| // Channel for receiving all the valid arguments | ||
| validArgsCh := make(chan string, len(args)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd call this argsCh.
cmd/dep/ensure.go
Outdated
| validArgsCh := make(chan string, len(args)) | ||
|
|
||
| // Channel for receiving all the validation errors | ||
| errArgsValidationCh := make(chan error, len(args)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd call this errsCh.
|
Apart from the variable names (and they are just an opinion) this LGTM and can be merged. |
Separating the validation code would enable testing the validation code in isolation.
9683e43 to
ebf6207
Compare
What does this do / why do we need it?
Performs
ensure -updatearguments validation concurrently.Related to TODOs:
What should your reviewer look out for in this PR?
Concurrent code implementation.