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

Add status feedback on memo mismatch #611

Merged
merged 2 commits into from
May 31, 2017
Merged

Add status feedback on memo mismatch #611

merged 2 commits into from
May 31, 2017

Conversation

darkowlzz
Copy link
Collaborator

@darkowlzz darkowlzz commented May 19, 2017 •
edited by sdboyer
Loading

  • Adds feedback to status when there's a lock memo mismatch. For
    mismatch with missing packages, it lists the packages and provides
    instructions to fix the issue. For memo mismatch with no missing
    packages, it instructs the user about how to generate a new lock.

Fixes #161 fixes #664

@darkowlzz
Copy link
Collaborator Author

status feedback on memo mismatch with missing packages:

$ dep status
Lock memo mismatch due to the following missing packages:

PROJECT                       MISSING PACKAGES
github.com/darkowlzz/openurl  [github.com/darkowlzz/openurl]

Run `dep ensure` to install the missing packages.

status feedback on memo mismatch without any missing packages:

$ dep status
Lock memo mismatch but there are no missing packages.
Run `dep ensure` to regenerate the lock memo.

@carolynvs
Copy link
Collaborator

It's not clear to me what is meant by "missing packages". I'm guessing you mean missing from vendor? It would help to put that in the message.

For the second case, when there's a mismatch but no "missing packages", unless missing packages is the only way to cause a mismatch it seems odd to mention that at all. I'd lean towards saying "Lock memo mismatch. Run dep ensure to regenerate the memo.".

@darkowlzz
Copy link
Collaborator Author

darkowlzz commented May 20, 2017 •
edited
Loading

It's not clear to me what is meant by "missing packages". I'm guessing you mean missing from vendor? It would help to put that in the message.

Yeah, being more specific in the message would be better. Right now, status doesn't checks what's in vendor/. Should it? It parses the project root, takes the manifest from file, computes a memo hash and compares it to the one found in lock file.
So the mismatch due to packages is when lock file has something missing from what's found in the package tree (addition of a new import) or package tree has something missing from what's in lock file (removal of an existing import).
And mismatch with no missing package is when the manifest constraints are modified.
But removal of existing import also falls under no missing package category, because the packages exist, just extra packages.

So, I think adding an extra line about the addition of new import would be better.

Lock memo mismatch due to the following missing packages:

PROJECT                       MISSING PACKAGES
github.com/darkowlzz/openurl  [github.com/darkowlzz/openurl]

This happens when a new import is added. Run `dep ensure` to install the missing packages.

when there's a mismatch but no "missing packages", unless missing packages is the only way to cause a mismatch it seems odd to mention that at all. I'd lean towards saying "Lock memo mismatch. Run dep ensure to regenerate the memo.".

Yeah, I think it's not required to mention "missing packages" here. But this can be made more informative like

Lock memo mismatch. This happens when Gopkg.toml is modified.
Run `dep ensure` to regenerate the memo.

Looks good?

@darkowlzz darkowlzz force-pushed the 161-status-memo-mismatch-feedback branch from 83681b5 to e525dc4 Compare May 20, 2017 08:01
@carolynvs
Copy link
Collaborator

Right now, status doesn't checks what's in vendor/. Should it?

Nope! 😄 We have another story for vendor validation.

So the mismatch due to packages is when lock file has something missing from what's found in the package tree (addition of a new import) or package tree has something missing from what's in lock file (removal of an existing import).

I pulled down your PR, correct me if I'm wrong, but your error messages don't cover the bolded case, right? So it's just the first scenario that this PR addresses? Just making sure I tested the right stuff.

carolynvs
carolynvs suggested changes May 22, 2017
View reviewed changes
cmd/dep/status.go Outdated
ctx.Loggers.Out.Print(buf.String())
if memoMismatch {
if hasMissingPkgs {
ctx.Loggers.Err.Println("Lock memo mismatch due to the following missing packages:\n")
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 clarify where the packages are missing, like Lock memo mismatch due to the following packages missing from the lock:

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 the message and squashed into a single commit.

@darkowlzz
Copy link
Collaborator Author

So it's just the first scenario that this PR addresses?

@carolynvs yes, just the first scenario, no missing packages, just extra packages.

@darkowlzz darkowlzz force-pushed the 161-status-memo-mismatch-feedback branch from e525dc4 to a5cbe76 Compare May 22, 2017 19:26
@carolynvs
Copy link
Collaborator

@sdboyer I was going to click the merge button for great glory but thought you'd like to weigh in since this PR was based on your feedback.

This was referenced May 27, 2017
Closed
Closed
@darkowlzz darkowlzz force-pushed the 161-status-memo-mismatch-feedback branch 2 times, most recently from 4cec5ac to b4e0ef0 Compare May 28, 2017 10:11
sdboyer
sdboyer suggested changes May 30, 2017
View reviewed changes
Copy link
Member

@sdboyer sdboyer left a comment

Choose a reason for hiding this comment

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

just one tiny tiny little spelling nit 😄

cmd/dep/status.go Outdated
@@ -400,7 +416,7 @@ func runStatusAll(loggers *dep.Loggers, out outputter, p *dep.Project, sm gps.So
loggers.Err.Printf("\t%s: %s\n", fail.ex, fail.err.Error())
}

return errors.New("address issues with undeducable import paths to get more status information.")
return memoMismatch, hasMissingPkgs, errors.New("address issues with undeducable import paths to get more status information.")
Copy link
Member

Choose a reason for hiding this comment

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

nit: s/undeducable/undeducible/

darkowlzz added 2 commits May 30, 2017 16:29
@darkowlzz
Add status feedback on memo mismatch
243be69 
- Adds feedback to status when there's a lock memo mismatch. For
mismatch with missing packages, it lists the packages and provides
instructions to fix the issue. For memo mismatch due to modified
manifest, it instructs the user about how to generate a new lock.
@darkowlzz
Rename memo to inputs-digest
6079300
@darkowlzz darkowlzz force-pushed the 161-status-memo-mismatch-feedback branch from b4e0ef0 to 6079300 Compare May 30, 2017 11:04
@darkowlzz
Copy link
Collaborator Author

Fixed the typo and squashed the commits.

@sdboyer
Copy link
Member

sdboyer commented May 31, 2017

thanks! 🎉

@sdboyer sdboyer merged commit 231ebf2 into golang:master May 31, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@carolynvs carolynvs carolynvs approved these changes

@sdboyer sdboyer sdboyer approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

status empty after change in Gopkg dep status is not useful if memo is out of date, not even a warning
4 participants
@darkowlzz @carolynvs @sdboyer @googlebot