-
Notifications
You must be signed in to change notification settings - Fork 23
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
Implement unencrypted ndt7 (+other maintainance changes) #31
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 6 files at r1.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @bassosimone and @pboothe)
cmd/ndt7-client/main.go, line 98 at r1 (raw file):
flagNoVerify = flag.Bool("no-verify", false, "skip TLS certificate verification") flagHostname = flag.String("hostname", "", "optional ndt7 server hostname") flagScheme = flag.String(
Consider making this a custom flag type to ensure that the scheme can only be one of the specified legal values?
Added one comment. If you don't want to do it, let me know and I will approve this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @pboothe)
cmd/ndt7-client/main.go, line 98 at r1 (raw file):
Previously, pboothe (Peter Boothe) wrote…
Consider making this a custom flag type to ensure that the scheme can only be one of the specified legal values?
Good idea! I'll try to use github.com/m-lab/flagx
for this purpose!
Allows to narrow down the allowed values to ws and wss as suggested by @pboothe during the review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 change requests, 0 of 1 approvals obtained
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 1 of 1 approvals obtained
This change is![Reviewable](https://camo.githubusercontent.com/23b05f5fb48215c989e92cc44cf6512512d083132bd3daf689867c8d9d386888/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)