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 all 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.Out, ctx.Verbose)
}

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)
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