Skip to content

ccli_test dial options for unencrypted gRPC and sending metadata#71

Merged
liulk merged 1 commit intoopenconfig:mainfrom
liulk:topic/cclidialopts
Aug 12, 2021
Merged

ccli_test dial options for unencrypted gRPC and sending metadata#71
liulk merged 1 commit intoopenconfig:mainfrom
liulk:topic/cclidialopts

Conversation

@liulk
Copy link
Copy Markdown
Contributor

@liulk liulk commented Aug 10, 2021

No description provided.

@liulk liulk requested a review from robshakir August 10, 2021 21:36
Copy link
Copy Markdown
Member

@robshakir robshakir left a comment

Choose a reason for hiding this comment

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

LGTM, a couple of minor suggestions.

Comment thread cmd/ccli/ccli_test.go Outdated
addr = flag.String("addr", "", "address of the gRIBI server in the format hostname:port")
addr = flag.String("addr", "", "address of the gRIBI server in the format hostname:port")
insecure = flag.Bool("insecure", false, "dial insecure gRPC (no TLS)")
skipverify = flag.Bool("skipverify", true, "allow self-signed TLS certificate")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(nit) Add something here that says that this is not required when "insecure" is also set?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good idea! Done.

Comment thread cmd/ccli/ccli_test.go
}, nil
}

func (flagCred) RequireTransportSecurity() bool {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks to be unused. I presume it's required to implement the argument for grpc.WithPerRPCCredentials? I'd suggest to add a comment to flagCred that just states what interface it is implementing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's right. Added comment to clarify.

@liulk liulk merged commit 8a7b2b5 into openconfig:main Aug 12, 2021
@liulk liulk deleted the topic/cclidialopts branch August 12, 2021 15:00
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