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

Initial version of test.proto #13

Closed
wants to merge 2 commits into from
Closed

Initial version of test.proto #13

wants to merge 2 commits into from

Conversation

tomek-US
Copy link
Contributor

No description provided.

Copy link
Contributor

@aghaffarkhah aghaffarkhah left a comment

Choose a reason for hiding this comment

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

Just a couple of small comments.

test/test.proto Outdated
// This gNOI service is a collection of operational RPC's that allow for the
// testing of a target by triggering events that otherwise would be difficult
// to trigger without sophisticated test-bed.
service StepExecutionService {
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets have only one service in this file which targets testing. So please pick a generic name like "Test" or something. This name is a bit non generic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

test/test.proto Outdated
// messages. Execution of each step is confirmed by transmitting back a
// StepExecutionResult message.
rpc ExecuteSteps(stream StepExecutionRequest)
returns (stream StepExecutionResult);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the format

rpc Foo (stream FooRequest) return (stream FooResponse)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

test/test.proto Outdated
// ExecuteSteps executes steps specified in the stream of StepExecutionRequest
// messages. Execution of each step is confirmed by transmitting back a
// StepExecutionResult message.
rpc ExecuteSteps(stream StepExecutionRequest)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just call this Execute and messages ExecuteRequet and ExecuteResponse? Seems more generic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

@marcushines marcushines closed this Aug 8, 2024
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