-
Notifications
You must be signed in to change notification settings - Fork 28
use specific error for github authorization error #493
use specific error for github authorization error #493
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I guess this test https://github.com/eclipse/codewind-installer/blob/master/pkg/project/create_test.go#L101 doesn't fail because your change is to the err op code not error.Desc. If so, it miight be good to add an assertion against the err op code
@rwalle61 - that's correct. I can add that assertion 👍 |
f05a0d7
to
a19d4ae
Compare
Signed-off-by: James Cockbain <[email protected]>
a19d4ae
to
20bdfb5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except for this last thing
pkg/project/create_test.go
Outdated
@@ -98,7 +98,8 @@ func TestDownloadTemplate(t *testing.T) { | |||
out, err := DownloadTemplate(dest, url, gitCredentials) | |||
|
|||
assert.Nil(t, out) | |||
assert.Equal(t, "unexpected status code: 401 Unauthorized", err.Desc) | |||
assert.Equal(t, err.Op, errOpInvalidCredentials) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi I recently realised that the 2nd arg of assert.Equal
should be the expected value, not the actual value. I.e. this (and the others) should be:
assert.Equal(t, err.Op, errOpInvalidCredentials) | |
assert.Equal(t, errOpInvalidCredentials, err.Op) |
which is a bit confusing given that assert.Contains(t, string(out), "invalid_git_credentials")
is the correct way round
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, yeah you are right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the change, yea I only just recently noticed it #489
Signed-off-by: James Cockbain <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Codecov Report
@@ Coverage Diff @@
## master #493 +/- ##
==========================================
+ Coverage 19.83% 19.86% +0.03%
==========================================
Files 80 80
Lines 7014 7017 +3
==========================================
+ Hits 1391 1394 +3
Misses 5453 5453
Partials 170 170
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Signed-off-by: James Cockbain [email protected]
What type of PR is this ?
What does this PR do ?
Uses a specific error key (
invalid_git_credentials
) for whenever the Github API returns a401 unauthorized
error, when creating a project.Example:
Modifies main tests to check for this error code in its existing bad password and access token tests.
Which issue(s) does this PR fix ?
eclipse-archived/codewind#3100
Does this PR require a documentation change ?
No
Any special notes for your reviewer ?
Modified tests require GHE credentials to be added in https://github.com/eclipse/codewind-installer/blob/master/pkg/test/globals.go.