Feature/keychain support

[ghost]

I've taken the PR branch from @jspc in #51 and rebased on current master.

Then I've addressed the issues discussed in #51 and addressed them here and pushed them as a new PR since I lack permission to modify an exiting PR.

[ghost]

@johananl I had a conflict on Gopkg.lock which I'm not sure if I resolved that right.

[ghost]

Can you please rebase over latest master?

Just did that.

[johananl]

Yeah, my bad. Had an older branch with the same name.

[johananl]

@johananl I had a conflict on Gopkg.lock which I'm not sure if I resolved that right.

You can run dep ensure to verify. It will change things in Gopkg.lock if necessary.

[ghost]

@johananl please review, too. thanks.

[ghost]

@johananl I think that should make it into the next release too? What do you think?

[ghost]

I've rebased over current master. Please take a look here and comment or fix what you don't like.

[pullrequest]

✅ A review job has been created and sent to the PullRequest network.

---

Check the status or cancel PullRequest code review here - or - cancel by adding [!pr] to the title of the pull request.

[pullrequest]

Welcome to PullRequest! Nice job abstracting this into an interface based on the windows/unix platforms. Overall this code looks pretty good, but I left a few different comments related to the error handling that could improve debugging and consistency. Feel free to reply inline if you have comments or suggestions.

---

Was this helpful? Yes | No

[johananl]

I see multiple problems with this PR (the main one is that it doesn't build), however looks like this has been merged already so I don't have a lot left to do :-/

[ghost]

@johananl ok, screwed that one up. Fixed in #77.

So what other issues do you see? care to elaborate?

[johananl]

The error isn't handled here. This makes the app attempt to authenticate even when now password was given or when Ctrl+C is pressed at the password prompt.

[ghost]

Why does it need to be handled here? What you are describing is handled in keychain/keychain.go where it's actually exported.

[ghost]

Lines 39 - 44 to be precise.

[johananl]

In Go there are no exceptions. Every time you call a function which returns an error, you should check if the error is not nil. Even if you got a valid result back from the function, you could also get an error at the same time. This is perfectly valid in Go.

In this specific case, the app's flow continues after pressing Ctrl+C for example at the password prompt. This could lead to unexpected results and strange user experience.

[johananl]

What you are describing is handled in keychain/keychain.go where it's actually exported.

This is not enough - if function C calls function B and function B calls function A and function A returns an error, you should have an error handling "chain":

func A() error {
	return errors.New("Oh no!")
}

func B() error {
	err := A()
	if err != nil {
		return err
	}
}

func C() {
	err := B()
	if err != nil {
		log.Fatalf("Something broke")
	}
}

[johananl]

The bottom line is - every time you have something like this:

err := whatever()

The following line should nearly always be:

if err != nil {
    // Do something.
}

[johananl]

I'll elaborate more when I can walk through this thoroughly and actually test the new features, but what I've seen from a brief review:

• There is an unhandled error. I commented inline.
• There is inconsistent capitalization in multiple places.
• Some user messages and documentation changes could be phrased better.

I'll do my best to review this thoroughly ASAP.
Shall I open a new PR with suggested changes for your review instead?

[ghost]

Yes, please open a new PR, I think that makes it easier to track.

I've commented on your inline comment.