New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add fail message for non-existent hostname #2785
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.
Thank you! The error message text needs changing, though.
Did you consider also making a test in status_test.go
?
pkg/cmd/auth/status/status.go
Outdated
@@ -139,6 +141,12 @@ func statusRun(opts *StatusOptions) error { | |||
// not to since I wanted this command to be read-only. | |||
} | |||
|
|||
if !isHostnameFound { | |||
fmt.Fprintf(stderr, | |||
"You are not logged into any GitHub hosts. Run %s to authenticate.\n", cs.Bold(fmt.Sprintf("gh auth login -h %s", opts.Hostname))) |
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.
We already print this message above:
cli/pkg/cmd/auth/status/status.go
Lines 71 to 74 in 4750e4a
if len(hostnames) == 0 || err != nil { | |
fmt.Fprintf(stderr, | |
"You are not logged into any GitHub hosts. Run %s to authenticate.\n", cs.Bold("gh auth login")) | |
return cmdutil.SilentError |
The message you added here isn't accurate for the condition we're testing. The issue you've set to fix is to warn that a hostname passed via the -h
argument wasn't found. How about something like this?
"Hostname %q not found among authenticated GitHub hosts"
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.
@mislav It seems that the code in status_test.go
silently ignores the stderr
message in favour of wantErr
.
cli/pkg/cmd/auth/status/status_test.go
Lines 239 to 243 in 4750e4a
assert.Equal(t, tt.wantErr == nil, err == nil) | |
if err != nil { | |
if tt.wantErr != nil { | |
assert.True(t, tt.wantErr.MatchString(err.Error())) | |
return |
Since we return SilentError
we have to make wantErr
non-nil, and that forces condition on L241 to occur, thus pre-maturely returning, avoiding a check on wantErrOut
. Any suggestions around this?
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.
This looks good! Thank you.
@samcoe to fix conflicts and merge |
Fixes #2775.