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

added instancesdk.Get() feature and its gRPC API #1075

Merged
merged 3 commits into from
Jun 21, 2019
Merged

Conversation

ilgooz
Copy link
Contributor

@ilgooz ilgooz commented Jun 19, 2019

depends on #1074

@ilgooz ilgooz requested a review from a team June 19, 2019 17:53
package api;

service Instance {
rpc Get (GetInstanceRequest) returns (GetInstanceResponse) {}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
rpc Get (GetInstanceRequest) returns (GetInstanceResponse) {}
rpc Get (GetInstanceRequest) returns (definition.Instance) {}

I really recommend to directly returning the Instance definition. That's the directly on the new API. Simpler is better.
Check:

service Execution {
// Get returns a single Execution specified in a request.
rpc Get(GetExecutionRequest) returns (definition.Execution) {}
// Stream returns a stream of executions that satisfy criteria
// specified in a request.
rpc Stream(StreamExecutionRequest) returns (stream definition.Execution) {}

Copy link
Member

Choose a reason for hiding this comment

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

Also, CreateInstanceResponse should be replaced also by the instance proto definition:

message CreateInstanceResponse {
string hash = 1; // Service's instance hash.
string serviceHash = 2; // Service's bare hash.
}

monday task: https://mesg.monday.com/boards/231078139/pulses/231079102/posts/361694557

Copy link
Contributor Author

@ilgooz ilgooz Jun 20, 2019

Choose a reason for hiding this comment

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

I really recommend to directly returning the Instance definition. That's the directly on the new API. Simpler is better.
Check:

service Execution {
// Get returns a single Execution specified in a request.
rpc Get(GetExecutionRequest) returns (definition.Execution) {}
// Stream returns a stream of executions that satisfy criteria
// specified in a request.
rpc Stream(StreamExecutionRequest) returns (stream definition.Execution) {}

I don't agree with this one as discussed before but I can commit the changes as you like, if you prefer that way.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, CreateInstanceResponse should be replaced also by the instance proto definition:

message CreateInstanceResponse {
string hash = 1; // Service's instance hash.
string serviceHash = 2; // Service's bare hash.
}

monday task: https://mesg.monday.com/boards/231078139/pulses/231079102/posts/361694557

done

@antho1404 antho1404 merged commit 200d7e3 into dev Jun 21, 2019
@antho1404 antho1404 deleted the feature/instance-get branch June 21, 2019 11:28
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.

3 participants