Skip to content
This repository has been archived by the owner on Sep 9, 2020. It is now read-only.

fix(ensure): concurrent update args validation #1175

Merged

Conversation

darkowlzz
Copy link
Collaborator

What does this do / why do we need it?

Performs ensure -update arguments validation concurrently.
Related to TODOs:

What should your reviewer look out for in this PR?

Concurrent code implementation.

// Log all the errors
if len(errArgsValidationCh) > 0 {
for err := range errArgsValidationCh {
ctx.Err.Println(err.Error())
Copy link
Collaborator

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:

@darkowlzz darkowlzz force-pushed the concurrent-ensure-update-validation branch from cdfe942 to 9683e43 Compare September 20, 2017 11:29
@darkowlzz
Copy link
Collaborator Author

Made the error output similar to manifest validation output with :

$ dep ensure -update github.com/go-yaml/yaml@master github.com/darkowlzz/openurl github.com/sdboyer/deptest/foo
Invalid arguments passed to ensure -update:

  ✗ github.com/sdboyer/deptest/foo is not a project root, try github.com/sdboyer/deptest instead
  ✗ github.com/darkowlzz/openurl is not present in Gopkg.lock, cannot -update it
  ✗ version constraint master passed for github.com/go-yaml/yaml, but -update follows constraints declared in Gopkg.toml, not CLI arguments

update arguments validation failed

Copy link
Collaborator

@ibrasho ibrasho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM


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))
Copy link
Collaborator

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.

validArgsCh := make(chan string, len(args))

// Channel for receiving all the validation errors
errArgsValidationCh := make(chan error, len(args))
Copy link
Collaborator

@ibrasho ibrasho Sep 21, 2017

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.

@ibrasho
Copy link
Collaborator

ibrasho commented Sep 21, 2017

Apart from the variable names (and they are just an opinion) this LGTM and can be merged.

@darkowlzz darkowlzz force-pushed the concurrent-ensure-update-validation branch from 9683e43 to ebf6207 Compare September 22, 2017 11:54
@darkowlzz darkowlzz merged commit c253f7e into golang:master Sep 22, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants