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

Ensure -vendor-only ignores -v #963

Closed
lwc opened this issue Aug 7, 2017 · 4 comments · Fixed by #968
Closed

Ensure -vendor-only ignores -v #963

lwc opened this issue Aug 7, 2017 · 4 comments · Fixed by #968

Comments

@lwc
Copy link

lwc commented Aug 7, 2017

Thanks so much for adding -vendor-only - this is a much needed feature for guaranteeing reliable CI builds 👍

What version of Go (go version) and dep (git describe --tags) are you using?

$ go version
go version go1.8.1 darwin/amd64

$ git describe --tags
v0.3.0-19-g52ed251

What dep command did you run?

Looks like -vendor-only doesn't have a verbose mode:

$ dep ensure -vendor-only
$ dep ensure -v -vendor-only
$

What did you expect to see?

Some kind of indication that something is happening - a list of what is getting installed?

What did you see instead?

Nada! 😁

@sdboyer
Copy link
Member

sdboyer commented Aug 7, 2017

hi, welcome! thanks for the issue 😄

Thanks so much for adding -vendor-only - this is a much needed feature for guaranteeing reliable CI builds 👍

🎉🎉 - sorry it took so long!

Some kind of indication that something is happening - a list of what is getting installed?

HAH! this literally never even occurred to me during the design.

-vendor-only basically just skips the first half of ensure's work, jumping right to the part where we write vendor/. that segment has never produced any feedback, which has (i guess) been mostly fine, but it's particularly obvious now that -vendor-only allows you to skip ahead.

achieving this probably wouldn't be too awful. i don't think it would be terribly useful right now, as we always and unconditionally regenerate vendor/ in its entirety on every dep ensure. however, we're well on our way - #959 - to introducing a system that will allow us to do only partial writes to vendor, at which point i think such feedback is likely to become much more useful.

for now, we should take the same basic approach as with gps.SolveParameters - optionally inject a logger into gps.WriteDepTree() (the func that populates vendor/), and incrementally report progress to that. SafeWriter.Write() will also then need to take a logger argument, the presence of which it can use as an indication that it should report its own progress.

@carolynvs you've worked on those parts a bunch - does that seem sane? could that interleave sanely with existing dep init output?

@ebati
Copy link
Contributor

ebati commented Aug 7, 2017

If you can assist me by answering questions below I can work on this issue.

  • We can create a global no-op logger and every function can use it. If verbose flag is set defaultlogger can be set to ctx.Out.
  • logger can be set optionally after creating SafeWriter. That way no signature change is needed in the public api ( gps.writeDepTree also needs signature change but it is an internal package ) .
  • logger can be sent optionally to sw.Write and from there to gps.writeDepTree
  • with dry-run and verbose selected together sw.PrintPreparedActions can be called in addition to vendor would be populated log.

@sdboyer
Copy link
Member

sdboyer commented Aug 7, 2017

If you can assist me by answering questions below I can work on this issue.

awesome, sure!

We can create a global no-op logger and every function can use it.

nope nope nope, no globals 😜

logger can be set optionally after creating SafeWriter. That way no signature change is needed in the public api

we're not worrying about public API stability yet, as we're pre-1.0.0. so it'll be fine to inject the desired *log.Logger (passed from *dep.Ctx) as a new parameter to sw.Write().

with dry-run and verbose selected together sw.PrintPreparedActions can be called in addition to vendor would be populated log.

hmm - we may actually need to make dry run a more explicit concept for SafeWriter, as we're probably going to need to pass that flag down to gps.WriteDepTree() as well.

@carolynvs
Copy link
Collaborator

dep init doesn't write until the very end, so it should play just fine with the existing output.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants