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

ensure: tweaks to align no-vendor behavior and verbose/dry-run logging #1039

Merged
merged 3 commits into from
Aug 21, 2017
Merged

ensure: tweaks to align no-vendor behavior and verbose/dry-run logging #1039

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5e271ca
ensure: tweaks to align no-vendor behavior and verbose/dry-run logging
jmank88 Aug 19, 2017
02031d9
ensure: dry-run: always log lock diff
jmank88 Aug 20, 2017
2f2d4e5
log PrintPreparedActions to ctx Out not Err
jmank88 Aug 20, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 14 additions & 18 deletions cmd/dep/ensure.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,15 +223,15 @@ func (cmd *ensureCommand) runDefault(ctx *dep.Ctx, args []string, p *dep.Project

if p.Lock != nil && bytes.Equal(p.Lock.InputHash(), solver.HashInputs()) {
// Memo matches, so there's probably nothing to do.
if ctx.Verbose {
ctx.Out.Printf("%s was already in sync with imports and %s\n", dep.LockName, dep.ManifestName)
}

if cmd.noVendor {
// The user said not to touch vendor/, so definitely nothing to do.
return nil
}

if ctx.Verbose {
ctx.Out.Printf("%s was already in sync with imports and %s, recreating vendor/ directory", dep.LockName, dep.ManifestName)
}

// TODO(sdboyer) The desired behavior at this point is to determine
// whether it's necessary to write out vendor, or if it's already
// consistent with the lock. However, we haven't yet determined what
Expand All @@ -244,8 +244,7 @@ func (cmd *ensureCommand) runDefault(ctx *dep.Ctx, args []string, p *dep.Project
}

if cmd.dryRun {
ctx.Out.Printf("Would have populated vendor/ directory from %s", dep.LockName)
return nil
return sw.PrintPreparedActions(ctx.Out, ctx.Verbose)
}

logger := ctx.Err
Expand All @@ -261,12 +260,16 @@ func (cmd *ensureCommand) runDefault(ctx *dep.Ctx, args []string, p *dep.Project
return errors.Wrap(err, "ensure Solve()")
}

sw, err := dep.NewSafeWriter(nil, p.Lock, dep.LockFromSolution(solution), dep.VendorOnChanged)
vendorBehavior := dep.VendorOnChanged
if cmd.noVendor {
vendorBehavior = dep.VendorNever
}
sw, err := dep.NewSafeWriter(nil, p.Lock, dep.LockFromSolution(solution), vendorBehavior)
if err != nil {
return err
}
if cmd.dryRun {
return sw.PrintPreparedActions(ctx.Out)
return sw.PrintPreparedActions(ctx.Out, ctx.Verbose)
}

logger := ctx.Err
Expand All @@ -292,14 +295,7 @@ func (cmd *ensureCommand) runVendorOnly(ctx *dep.Ctx, args []string, p *dep.Proj
}

if cmd.dryRun {
ctx.Out.Printf("Would have populated vendor/ directory from %s", dep.LockName)
if ctx.Verbose {
err := sw.PrintPreparedActions(ctx.Err)
if err != nil {
return errors.WithMessage(err, "prepared actions")
}
}
return nil
return sw.PrintPreparedActions(ctx.Err, ctx.Verbose)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the only one to pass ctx.Err, but I don't see a reason to.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed to ctx.Out to align with others.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that we've been logging info to stdout and verbose to stderr?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see that now in some other places, but maybe we need an issue to document the policy and audit/align the existing usages.

}

logger := ctx.Err
Expand Down Expand Up @@ -394,7 +390,7 @@ func (cmd *ensureCommand) runUpdate(ctx *dep.Ctx, args []string, p *dep.Project,
return err
}
if cmd.dryRun {
return sw.PrintPreparedActions(ctx.Out)
return sw.PrintPreparedActions(ctx.Out, ctx.Verbose)
}

logger := ctx.Err
Expand Down Expand Up @@ -648,7 +644,7 @@ func (cmd *ensureCommand) runAdd(ctx *dep.Ctx, args []string, p *dep.Project, sm
}

if cmd.dryRun {
return sw.PrintPreparedActions(ctx.Out)
return sw.PrintPreparedActions(ctx.Out, ctx.Verbose)
}

logger := ctx.Err
Expand Down
38 changes: 24 additions & 14 deletions txn_writer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,24 +418,30 @@ fail:
}

// PrintPreparedActions logs the actions a call to Write would perform.
func (sw *SafeWriter) PrintPreparedActions(output *log.Logger) error {
func (sw *SafeWriter) PrintPreparedActions(output *log.Logger, verbose bool) error {
if sw.HasManifest() {
output.Printf("Would have written the following %s:\n", ManifestName)
m, err := sw.Manifest.MarshalTOML()
if err != nil {
return errors.Wrap(err, "ensure DryRun cannot serialize manifest")
if verbose {
m, err := sw.Manifest.MarshalTOML()
if err != nil {
return errors.Wrap(err, "ensure DryRun cannot serialize manifest")
Copy link
Collaborator

@ibrasho ibrasho Aug 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure that's an appropriate error message.

Could we extract the "ensure DryRun " part to the caller? (same comment of the errors below)

return errors.Wrap(err, "cannot serialize manifest")

and

errors.Wrap(sw.PrintPreparedActions(ctx.Out, ctx.Verbose), "ensure dry run failed")

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure that's an appropriate error message.

I'm inclined to agree, but I didn't want too much to creep into this PR.

I like striping out "ensure dry run". Does it even need to be said at all, or could it be left out of both levels?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We have stuff like that in our messages in places where we weren't consistently using the errors package and it helped narrow down where the messages were coming from.

I've been removing these "it broke here" type messages, and either making sure that the original error had a good message and stack trace (i.e. it uses the errors package), or calling wrap and writing a message that uses full sentences that could be printed to the console and still make sense. 😁

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's do this in another PR.

}
output.Printf("Would have written the following %s:\n%s\n", ManifestName, string(m))
} else {
output.Printf("Would have written %s.\n", ManifestName)
}
output.Println(string(m))
}

if sw.writeLock {
if sw.lockDiff == nil {
output.Printf("Would have written the following %s:\n", LockName)
l, err := sw.lock.MarshalTOML()
if err != nil {
return errors.Wrap(err, "ensure DryRun cannot serialize lock")
if verbose {
l, err := sw.lock.MarshalTOML()
if err != nil {
return errors.Wrap(err, "ensure DryRun cannot serialize lock")
}
output.Printf("Would have written the following %s:\n%s\n", LockName, string(l))
} else {
output.Printf("Would have written %s.\n", LockName)
}
output.Println(string(l))
} else {
output.Printf("Would have written the following changes to %s:\n", LockName)
diff, err := formatLockDiff(*sw.lockDiff)
Copy link
Collaborator

@carolynvs carolynvs Aug 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The code is no longer printing the lock diff when -v is not specified which seems a little overly terse. I prefer that it is always printed during a dry-run, not just on verbose.

Otherwise I either have to settle for no useful output (other than knowing that dep found changes), or I have to wade through dozens of lines of solve output just to see what new package would be written to my lock. 😀

e.g. Terse

$ dep ensure -dry-run
Would have written changes to Gopkg.lock.
Would have written 16 projects to the vendor directory.

Verbose

$ dep ensure -dry-run -v
Root project is "github.com/meyskens/readmyage-api"
 3 transitively valid internal packages
 6 external packages imported from 5 projects
(0)   ✓ select (root)
(1)	? attempt github.com/labstack/echo with 2 pkgs; at least 1 versions to try
(1)	    try github.com/labstack/[email protected]
(1)	✓ select github.com/labstack/[email protected] w/2 pkgs
(2)	? attempt github.com/dgrijalva/jwt-go with 1 pkgs; at least 1 versions to try
(2)	    try github.com/dgrijalva/[email protected]
(2)	✓ select github.com/dgrijalva/[email protected] w/1 pkgs
(3)	? attempt github.com/stretchr/testify with 1 pkgs; at least 1 versions to try
(3)	    try github.com/stretchr/[email protected]
(3)	✓ select github.com/stretchr/[email protected] w/1 pkgs
(4)	? attempt github.com/davecgh/go-spew with 1 pkgs; at least 1 versions to try
(4)	    try github.com/davecgh/[email protected]
(4)	✓ select github.com/davecgh/[email protected] w/1 pkgs
(5)	? attempt golang.org/x/crypto with 1 pkgs; at least 1 versions to try
(5)	    try golang.org/x/crypto@master
(5)	✓ select golang.org/x/crypto@master w/2 pkgs
(6)	? attempt github.com/labstack/gommon with 4 pkgs; at least 1 versions to try
(6)	    try github.com/labstack/[email protected]
(6)	✓ select github.com/labstack/[email protected] w/4 pkgs
(7)	? attempt github.com/mattn/go-colorable with 1 pkgs; at least 1 versions to try
(7)	    try github.com/mattn/[email protected]
(7)	✓ select github.com/mattn/[email protected] w/1 pkgs
(8)	? attempt gopkg.in/resty.v0 with 1 pkgs; at least 1 versions to try
(8)	    try gopkg.in/[email protected]
(8)	✓ select gopkg.in/[email protected] w/1 pkgs
(9)	? attempt github.com/mattn/go-isatty with 1 pkgs; at least 1 versions to try
(9)	    try github.com/mattn/[email protected]
(9)	✓ select github.com/mattn/[email protected] w/1 pkgs
(10)  ? attempt github.com/pmezard/go-difflib with 1 pkgs; at least 1 versions to try
(10)      try github.com/pmezard/[email protected]
(10)  ✓ select github.com/pmezard/[email protected] w/1 pkgs
(11)  ? attempt github.com/valyala/fasttemplate with 1 pkgs; at least 1 versions to try
(11)      try github.com/valyala/fasttemplate@master
(11)  ✓ select github.com/valyala/fasttemplate@master w/1 pkgs
(12)  ? attempt github.com/valyala/bytebufferpool with 1 pkgs; at least 1 versions to try
(12)      try github.com/valyala/bytebufferpool@master
(12)  ✓ select github.com/valyala/bytebufferpool@master w/1 pkgs
(13)  ? attempt github.com/sdboyer/deptest with 1 pkgs; 4 versions to try
(13)      try github.com/sdboyer/[email protected]
(13)  ✓ select github.com/sdboyer/[email protected] w/1 pkgs
(14)  ? attempt golang.org/x/sys with 1 pkgs; at least 1 versions to try
(14)      try golang.org/x/sys@master
(14)  ✓ select golang.org/x/sys@master w/1 pkgs
(15)  ? attempt golang.org/x/net with 1 pkgs; at least 1 versions to try
(15)      try golang.org/x/net@master
(15)  ✓ select golang.org/x/net@master w/2 pkgs
(16)  ? attempt golang.org/x/text with 2 pkgs; at least 1 versions to try
(16)      try golang.org/x/text@master
(16)  ✓ select golang.org/x/text@master w/9 pkgs
  ✓ found solution with 30 packages from 16 projects

Solver wall times by segment:
     b-source-exists: 3.943060411s
         b-list-pkgs: 3.493432407s
              b-gmal: 3.359844321s
  b-deduce-proj-root:  97.322674ms
             satisfy:   4.855499ms
         select-atom:   4.218009ms
            new-atom:   1.096136ms
         select-root:    151.083µs
               other:     74.229µs
     b-list-versions:     26.213µs

  TOTAL: 10.904080982s

Would have written the following changes to Gopkg.lock:
Memo: 4de59b7a3096f65ac52284a5a5e07c3ee048c47d67893392d1c8f2f409cfc98f -> 229dbcd910e23d4cc7f9713836f3243a12c5029b594ad68bc4dc78623ec26349

Add:
[[projects]]
  name = "github.com/sdboyer/deptest"
  packages = ["."]
  revision = "ff2948a2ac8f538c4ecd55962e919d1e13e74baf"
  version = "v1.0.0"


Would have written the following 16 projects to the vendor directory:
github.com/labstack/[email protected] with packages: [. middleware]
github.com/dgrijalva/[email protected] with packages: [.]
github.com/davecgh/[email protected] with packages: [spew]
github.com/mattn/[email protected] with packages: [.]
github.com/mattn/[email protected] with packages: [.]
github.com/pmezard/[email protected] with packages: [difflib]
github.com/valyala/fasttemplate@master with packages: [.]
github.com/stretchr/[email protected] with packages: [assert]
golang.org/x/crypto@master with packages: [acme acme/autocert]
github.com/labstack/[email protected] with packages: [bytes color log random]
github.com/sdboyer/[email protected] with packages: [.]
golang.org/x/sys@master with packages: [unix]
gopkg.in/[email protected] with packages: [.]
github.com/valyala/bytebufferpool@master with packages: [.]
golang.org/x/net@master with packages: [idna publicsuffix]
golang.org/x/text@master with packages: [internal/gen internal/triegen internal/ucd secure/bidirule transform unicode/bidi unicode/cldr unicode/norm unicode/rangetable]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fine with me. Should this apply to dry-run for all sub commands? Most were already behaving this way, but:

  • runVendorOnly was doing its own verbosity switch
  • runDefault with a synced lock never logged verbose

Copy link
Collaborator

Choose a reason for hiding this comment

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

  • In case of a synced lock, there shouldn't be any diff to print.
  • For vendor only, again, it's not modifying the lock so there's no need to print the diff.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was thinking about reverting the whole method, so manifest and vendor in addition to the lock.
Are you proposing that only the lock diff be always logged, while manifest and vendor details would remain verbose only?

Copy link
Collaborator

@carolynvs carolynvs Aug 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I definitely think that the vendor info e.g. github.com/stretchr/[email protected] with packages: [assert] should only be logged in verbose mode, since we don't have vendor verification and therefore every package is listed regardless of what's changed.

Not so sure about the manifest though. I think it will print the entire manifest contents, even when only 1 thing has changed, right? Basically we don't have manifest diffing? If we don't have manifest diffing, and there's no issue for that, it would be great to make a new issue for that! 😁

From a user perspective, I'd prefer to only see changes when I ask for a dry-run, and put anything that is a "mega dump" under the verbose flag.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated

Expand All @@ -447,9 +453,13 @@ func (sw *SafeWriter) PrintPreparedActions(output *log.Logger) error {
}

if sw.writeVendor {
output.Println("Would have written the following projects to the vendor directory:")
for _, project := range sw.lock.Projects() {
output.Println(project)
if verbose {
output.Printf("Would have written the following %d projects to the vendor directory:\n", len(sw.lock.Projects()))
for _, project := range sw.lock.Projects() {
output.Println(project)
Copy link
Collaborator

@ibrasho ibrasho Aug 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Shouldn't we prepend anything here like * ?

Copy link
Collaborator Author

@jmank88 jmank88 Aug 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Perhaps another candidate for counts, like in #1037. I can include this one in a future PR.

}
} else {
output.Printf("Would have written %d projects to the vendor directory.\n", len(sw.lock.Projects()))
}
}

Expand Down