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

fix(status): check lockfile before runStatusAll() #1238

Merged
merged 2 commits into from
Oct 7, 2017

Conversation

darkowlzz
Copy link
Collaborator

What does this do / why do we need it?

Moves lockfile check before runStatusAll().

What should your reviewer look out for in this PR?

¯_(ツ)_/¯

Do you need help or clarification on anything?

No.

Which issue(s) does this PR fix?

fixes #1232

Copy link
Collaborator

@jmank88 jmank88 left a comment

Choose a reason for hiding this comment

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

LGTM

Isn't this missing check another separate problem affecting #1232?

@darkowlzz
Copy link
Collaborator Author

@jmank88 err... I think that link is pointing to code that has changed recently 😅 Can you check if there's still a problem?

@darkowlzz
Copy link
Collaborator Author

oh! I see an issue, maybe the same issue you're referring, even in the new code. The error is received but never logged but Failed to get status. Rerun with -v flag to see details. is printed.
Maybe we should just log the received error in default in verbose mode.

ctx.Err.Println(err)
} else {
ctx.Out.Println("Failed to get status. Rerun with `-v` flag to see details.")
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was only thinking "don't suggest -v when it's already present". Isn't the same err already returned and logged higher up somewhere?

Copy link
Collaborator Author

@darkowlzz darkowlzz Oct 4, 2017

Choose a reason for hiding this comment

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

oops! you're right, I overlooked that 😅 So, we get rid of the default part and there's no need to move the lock check. But moving the check up there doesn't harm in anyway. Let's keep it up there.

@sdboyer sdboyer merged commit a4c6879 into golang:master Oct 7, 2017
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dep status fails, tells me to rerun with -v, which gives me the same message
5 participants