ensure: tweaks to align no-vendor behavior and verbose/dry-run logging #1039
Conversation
txn_writer.go
Outdated
| } else { | ||
| output.Printf("Would have written the following changes to %s:\n", LockName) | ||
| diff, err := formatLockDiff(*sw.lockDiff) |
There was a problem hiding this comment.
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/echo@v3.2.1
(1) ✓ select github.com/labstack/echo@v3.2.1 w/2 pkgs
(2) ? attempt github.com/dgrijalva/jwt-go with 1 pkgs; at least 1 versions to try
(2) try github.com/dgrijalva/jwt-go@v3.0.0
(2) ✓ select github.com/dgrijalva/jwt-go@v3.0.0 w/1 pkgs
(3) ? attempt github.com/stretchr/testify with 1 pkgs; at least 1 versions to try
(3) try github.com/stretchr/testify@v1.1.4
(3) ✓ select github.com/stretchr/testify@v1.1.4 w/1 pkgs
(4) ? attempt github.com/davecgh/go-spew with 1 pkgs; at least 1 versions to try
(4) try github.com/davecgh/go-spew@v1.1.0
(4) ✓ select github.com/davecgh/go-spew@v1.1.0 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/gommon@0.2.2
(6) ✓ select github.com/labstack/gommon@0.2.2 w/4 pkgs
(7) ? attempt github.com/mattn/go-colorable with 1 pkgs; at least 1 versions to try
(7) try github.com/mattn/go-colorable@v0.0.9
(7) ✓ select github.com/mattn/go-colorable@v0.0.9 w/1 pkgs
(8) ? attempt gopkg.in/resty.v0 with 1 pkgs; at least 1 versions to try
(8) try gopkg.in/resty.v0@v0.13
(8) ✓ select gopkg.in/resty.v0@v0.13 w/1 pkgs
(9) ? attempt github.com/mattn/go-isatty with 1 pkgs; at least 1 versions to try
(9) try github.com/mattn/go-isatty@v0.0.2
(9) ✓ select github.com/mattn/go-isatty@v0.0.2 w/1 pkgs
(10) ? attempt github.com/pmezard/go-difflib with 1 pkgs; at least 1 versions to try
(10) try github.com/pmezard/go-difflib@v1.0.0
(10) ✓ select github.com/pmezard/go-difflib@v1.0.0 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/deptest@v1.0.0
(13) ✓ select github.com/sdboyer/deptest@v1.0.0 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/echo@v3.2.1 with packages: [. middleware]
github.com/dgrijalva/jwt-go@v3.0.0 with packages: [.]
github.com/davecgh/go-spew@v1.1.0 with packages: [spew]
github.com/mattn/go-colorable@v0.0.9 with packages: [.]
github.com/mattn/go-isatty@v0.0.2 with packages: [.]
github.com/pmezard/go-difflib@v1.0.0 with packages: [difflib]
github.com/valyala/fasttemplate@master with packages: [.]
github.com/stretchr/testify@v1.1.4 with packages: [assert]
golang.org/x/crypto@master with packages: [acme acme/autocert]
github.com/labstack/gommon@0.2.2 with packages: [bytes color log random]
github.com/sdboyer/deptest@v1.0.0 with packages: [.]
golang.org/x/sys@master with packages: [unix]
gopkg.in/resty.v0@v0.13 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]
There was a problem hiding this comment.
Fine with me. Should this apply to dry-run for all sub commands? Most were already behaving this way, but:
runVendorOnlywas doing its own verbosity switchrunDefaultwith a synced lock never logged verbose
There was a problem hiding this comment.
- 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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I definitely think that the vendor info e.g. github.com/stretchr/testify@v1.1.4 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.
cmd/dep/ensure.go
Outdated
| } | ||
| } | ||
| return nil | ||
| return sw.PrintPreparedActions(ctx.Err, ctx.Verbose) |
There was a problem hiding this comment.
This is the only one to pass ctx.Err, but I don't see a reason to.
There was a problem hiding this comment.
Changed to ctx.Out to align with others.
There was a problem hiding this comment.
I think that we've been logging info to stdout and verbose to stderr?
There was a problem hiding this comment.
I see that now in some other places, but maybe we need an issue to document the policy and audit/align the existing usages.
| if verbose { | ||
| m, err := sw.Manifest.MarshalTOML() | ||
| if err != nil { | ||
| return errors.Wrap(err, "ensure DryRun cannot serialize manifest") |
There was a problem hiding this comment.
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")?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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. 😁
There was a problem hiding this comment.
Let's do this in another PR.
| 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) |
There was a problem hiding this comment.
Shouldn't we prepend anything here like * ?
There was a problem hiding this comment.
Perhaps another candidate for counts, like in #1037. I can include this one in a future PR.
What does this do / why do we need it?
This PR proposes a few tweaks to the
ensurecommand, which intend only to increase consistency and orthogonality:no-vendor)no-vendoreven if the lock file was not already synceddry-runlogging viaPrintPreparedActionsusagesPrintPreparedActionsto handle verbosityWhat should your reviewer look out for in this PR?
Oversights, better log messages.