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.

Sign up for GitHub

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

Jump to bottom

Add Debug Operation #33

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Add Debug Operation #33

wants to merge 1 commit into from

Conversation

asafsonnv
Copy link
Contributor

@asafsonnv
Copy link
Contributor Author

Hi:)
Should we implement the Execute() for Debug() to send and receive messages from the server?
for sending I see some logic that sends that opens the stream and sends messages in debug_grpc.pb.go:
func (c *debugClient) Debug(ctx context.Context, in *DebugRequest, opts ...grpc.CallOption) (Debug_DebugClient, error) { stream, err := c.cc.NewStream(ctx, &Debug_ServiceDesc.Streams[0], "/gnoi.debug.Debug/Debug", opts...) if err != nil { return nil, err } x := &debugDebugClient{stream} if err := x.ClientStream.SendMsg(in); err != nil { return nil, err } if err := x.ClientStream.CloseSend(); err != nil { return nil, err } return x, nil }

currently the Execute() returns a DebugClient.

@asafsonnv
Copy link
Contributor Author

For the debug_test.go:
should I add a fake server to return a message?

@asafsonnv
Copy link
Contributor Author

@greg-dennis WDYT?

greg-dennis
greg-dennis reviewed Dec 3, 2024
View reviewed changes
Copy link
Contributor

@greg-dennis greg-dennis left a comment

Choose a reason for hiding this comment

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

I'm not involved in the project anymore, so I don't have any kind of meaningful say over this. I made a few small comments and overall it looks alright, but you'll have to leave it to the people who are involved to say for sure.

debug/debug.go
@@ -0,0 +1,81 @@
// Copyright 2023 Google LLC
Copy link
Contributor

Choose a reason for hiding this comment

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

2024

gnoigo.go
@@ -70,6 +72,7 @@ type Clients interface {
OTDR() otpb.OTDRClient
System() spb.SystemClient
WavelengthRouter() wrpb.WavelengthRouterClient
DEBUG() dbgpb.DebugClient
Copy link
Contributor

Choose a reason for hiding this comment

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

Should just be stylized as "Debug" not "DEBUG" because it isn't an intiailism.

internal/clients.go
@@ -112,3 +114,8 @@ func (c *Clients) System() spb.SystemClient {
func (c *Clients) WavelengthRouter() wrpb.WavelengthRouterClient {
return c.WavelengthRouterClient
}

// Debug returns the client for gNOI Debug service.
func (c *Clients) DEBUG() dbgpb.DebugClient {
Copy link
Contributor

Choose a reason for hiding this comment

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

"DEBUG" -> "Debug"

debug/debug.go

// Mode the commands will be executed in.
//
// enum Mode {
Copy link
Contributor

Choose a reason for hiding this comment

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

Putting the full enum in the comment creates a double-maintenance burden. I'd just let people look it up in the proto.

@asafsonnv
Copy link
Contributor Author

asafsonnv commented Dec 4, 2024 •
edited
Loading

@dplore Are you the current maintainer of this project?

@asafsonnv
Copy link
Contributor Author

@prinikasn may you can assist?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@greg-dennis greg-dennis greg-dennis left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@asafsonnv @greg-dennis