Skip to content
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

api: add gRPC definition for version endpoint. #683

Merged
merged 1 commit into from
Nov 14, 2016

Conversation

rithujohn191
Copy link
Contributor

Closes #659.

// Dex represents the dex gRPC service.
service Dex {
// CreateClient attempts to create the client.
// CreateClient creates the client.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: creates a client.

@rithujohn191
Copy link
Contributor Author

@ericchiang.

Small note: .proto requires you to decalre it as int32 or int64, there is no default int type. Chose int32 for now. let me know if you want to change it.

Copy link
Contributor

@ericchiang ericchiang left a comment

Choose a reason for hiding this comment

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

few nits

message VersionReq {}

// VersionResp returns version info.
message VersionResp {
Copy link
Contributor

Choose a reason for hiding this comment

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

This can probably just hold the Version fields directly. We don't need the Version message.

message Version {
// Semantic version of the server.
string server = 1;
// Numeric version of the API.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please expand this comment to reflect the comment in server/api.go. Maybe even just copy it here.

We expect outside users to read this file, not the Go code.

rpc DeletePassword(DeletePasswordReq) returns (DeletePasswordResp) {};
// GetVersion gets the version info.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe GetVersion returns version information of the server.

@ericchiang
Copy link
Contributor

Small note: .proto requires you to decalre it as int32 or int64, there is no default int type. Chose int32 for now. let me know if you want to change it.

int32 sounds fine. Doubt we'll run out of space.

@ericchiang
Copy link
Contributor

lgtm

@ericchiang ericchiang merged commit e1f6679 into dexidp:master Nov 14, 2016
@rithujohn191 rithujohn191 deleted the add-version-endpoint branch November 14, 2016 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants