New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/dep: always ensure that gps.SolveParameters are validated #981

Merged
merged 1 commit into from Aug 13, 2017

Conversation

Projects
None yet
5 participants
@ibrasho
Copy link
Collaborator

ibrasho commented Aug 9, 2017

Same as #697

Ensure that we validate gps.SolveParameters in dep init, dep status and the new modes in dep ensure.

@ibrasho ibrasho requested review from carolynvs and darkowlzz as code owners Aug 9, 2017

@googlebot googlebot added the cla: yes label Aug 9, 2017

@ibrasho ibrasho force-pushed the ibrasho-forks:ensure-packages-are-deducible-before-solving branch from daaffa7 to 186b556 Aug 9, 2017

@sdboyer

This comment has been minimized.

Copy link
Member

sdboyer commented Aug 9, 2017

would it be easy to add some basic regression tests for this, just via the test harness?

}
return errors.Wrap(err, "validateParams")
}

This comment has been minimized.

@darkowlzz

darkowlzz Aug 9, 2017

Collaborator

Any reason for repeating and not sharing the code with a common function?

This comment has been minimized.

@ibrasho

ibrasho Aug 11, 2017

Collaborator

I'll update this PR. 👍

@carolynvs
Copy link
Collaborator

carolynvs left a comment

I'm good with this, but agree with @darkowlzz that I'd prefer the validation to be a shared function.

cmd/dep: always ensure that gps.SolveParameters are validated
Signed-off-by: Ibrahim AshShohail <ibra.sho@gmail.com>

@ibrasho ibrasho force-pushed the ibrasho-forks:ensure-packages-are-deducible-before-solving branch from 186b556 to 56d7909 Aug 11, 2017

@ibrasho ibrasho requested a review from sdboyer as a code owner Aug 11, 2017

@ibrasho

This comment has been minimized.

Copy link
Collaborator

ibrasho commented Aug 11, 2017

@carolynvs, @darkowlzz : I've added a method in dep.Ctx but I'm honestly not sure if that's the most logical place for it. Any alternative suggestions?

}

return errors.Wrap(err, "validateParams")
}

This comment has been minimized.

@darkowlzz

darkowlzz Aug 12, 2017

Collaborator

Since all the parts involved in this are gps related, maybe adding it to gps/solver.go would be better, like ValidateParamsWithLogs() or some better name? Just a suggestion.

This comment has been minimized.

@ibrasho

ibrasho Aug 12, 2017

Collaborator

I'm open for suggestions.

The only reason I had to put it in dep.Ctx was that it's about logging the errors since the real logic is in gps.

This comment has been minimized.

@sdboyer

sdboyer Aug 13, 2017

Member

let's stick with this for now, at least. i know we're continuing to add more logging directly in gps elsewhere, but i still want to try to do that as little as possible, until i have time for some proper design think on how to do it generally.

@sdboyer sdboyer merged commit d13e867 into golang:master Aug 13, 2017

4 checks passed

cla/google All necessary CLAs are signed
codeclimate All good!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ibrasho ibrasho deleted the ibrasho-forks:ensure-packages-are-deducible-before-solving branch Nov 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment