-
Notifications
You must be signed in to change notification settings - Fork 277
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 buf registry whoami
command
#3416
Conversation
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
CHANGELOG.md
Outdated
@@ -2,7 +2,8 @@ | |||
|
|||
## [Unreleased] | |||
|
|||
- No changes yet. | |||
- Add `buf beta registry whoami` command, which checks if you are logged in to the Buf Schema |
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.
Should we have this start as a beta
command and then promote?
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.
No
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.
Moved this out of beta
and directly to buf registry whoami
.
if connectErr := new(connect.Error); errors.As(err, &connectErr) && connectErr.Code() == connect.CodeUnauthenticated { | ||
return fmt.Errorf("Not currently logged in for %s", remote) | ||
} |
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.
Checking for the unauthenticated error code here and returning this error, we get the following output:
$ buf beta registry whoami
Failure: Not currently logged in for buf.build
If we do not check, and simply return what the error wrapper gives us, we get:
$ buf beta registry whoami
Failure: you are not authenticated for buf.build. Set the BUF_TOKEN environment variable or run "buf registry login", using a Buf API token as the password. For details, visit https://buf.build/docs/bsr/authentication
Both clearly indicate to the user they are unauthenticated, so they both seem fine to me, it just depends on what we want the UX to be.
if connectErr := new(connect.Error); errors.As(err, &connectErr) && connectErr.Code() == connect.CodeUnauthenticated { | ||
return fmt.Errorf("Not currently logged in for %s", remote) | ||
} | ||
return err |
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.
Right now, we aren't really differentiating between unauthenticated vs. other error states, everything exits 1
unless authenticated (which exits 0
).
The error message will indicate the nature of the error, for example, in the case of an invalid remote:
$ buf beta registry whoami fake.address
Failure: the server hosted at that remote is unavailable. Are you sure "fake.address" is a valid remote address?
But we do not do any special handling for the errors.
type flags struct{} | ||
|
||
func newFlags() *flags { | ||
return &flags{} |
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.
What do you think about a --format=json
for accessing the username?
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.
I like this! I can add 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.
Added --format=json
(and --format=text
will print the "logged in as" statement as before).
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.
Probably good to go
@@ -248,6 +248,14 @@ func NewOrganizationEntity(organization *ownerv1.Organization, remote string) En | |||
} | |||
} | |||
|
|||
func NewUserEntity(user *registryv1alpha1.User) Entity { |
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.
No godoc
formatFlagName = "format" | ||
) | ||
|
||
func NewCommand( |
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.
No godoc
} | ||
user := currentUserResponse.Msg.User | ||
if user == nil { | ||
return errors.New("No user found for provided token") |
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.
What am I supposed to do with this as a user? What provided token? Where did the provided token come from? This error message doesn't help me much.
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 mirrors the error that is returned from buf registry login
when we check the token:
buf/private/buf/cmd/buf/command/registry/registrylogin/registrylogin.go
Lines 212 to 214 in 81698bb
if user == nil { | |
return errors.New("no user found for provided token") | |
} |
We can prompt the user to get a new token using buf registry login
. If that makes sense, then I can update the error message on both sides.
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.
I didn't provide a token though. Like as a user, this command does not accept a token, so it doesn't make sense to tell the user that their provided token didn't result in a user.
I get that this is the error message for the other one, but this just means both need to be updated, and should in no way reference tokens, as the user does not interact with tokens (typically) via either this command of buf registry login
.
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.
That is fair -- I left the buf registry login
error as-is, since it's part of a validation flow where the token is provided through the command. For this one, I provided instructions to the user to refresh their credentials and to check the env var if set.
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.
That doesn't make sense to me - the error message is just as confusing for buf registry login
.
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.
Still waiting on 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.
For buf registry login
, the error message comes up after the token is provided through the login process, so it should still make sense... in the case of buf registry login
, it might make sense to report it as a syserror, since the token is not provided by the user.
Adjust the title and description |
buf beta registry whoami
commandbuf registry whoami
command
user := currentUserResponse.Msg.User | ||
if user == nil { | ||
return fmt.Errorf( | ||
`No valid user found for login credentials. Run %q to refresh your credentials. If you have %s environment variable set, ensure that the token is valid.`, |
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.
- "If you have BUF_TOKEN environment variable set" isn't proper english, I believe you mean "If you have the BUF_TOKEN environment variable set".
- "Run 'buf registry login' to refresh your credentials" isn't correct. If someone runs "buf registry whoami acme.buf.dev", they should be instructed to run "buf registry login acme.buf.dev".
- Change "No valid user found for login credentials" to "No user is logged in to %s", with the hostname being put in there.
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.
Errors are now reported as:
$ buf registry whoami
Failure: No user is logged in to buf.build. Run "buf registry login" to refresh your credentials. If you have the BUF_TOKEN environment variable set, ensure that the token is valid.
exit status 1
$ buf registry whoami bufbuild.internal
Failure: No user is logged in to bufbuild.internal. Run "buf registry login bufbuild.internal" to refresh your credentials. If you have the BUF_TOKEN environment variable set, ensure that the token is valid.
exit status 1
This adds a
buf registry whoami
command, which checks if youare logged in to the Buf Schema Registry at a given domain. The domain
defaults
buf.build
if none is provided.Fixes #3414