nsq_to_http: add custom header flag #1072
Conversation
alwindoss
changed the title
apps/nsq_to_http/nsq_to_http.go
Outdated
| ) | ||
|
|
||
| func init() { | ||
| flag.Var(&postAddrs, "post", "HTTP address to make a POST request to. data will be in the body (may be given multiple times)") | ||
| flag.Var(&customHeaders, "custom-header", "Custom header for HTTP requests (may be given multiple times)") |
There was a problem hiding this comment.
I suggest making the flag shorter, just header (it would also match curl's --header flag)
There was a problem hiding this comment.
Agreed, this would enable people who are comfortable with curl to use this option
apps/nsq_to_http/nsq_to_http.go
Outdated
| func parseCustomHeaders(strs []string) (map[string]string, error) { | ||
| parsedHeaders := make(map[string]string) | ||
| for _, s := range strs { | ||
| sp := strings.Split(s, "=") |
There was a problem hiding this comment.
For the curl --header flag previously mentioned, one passes the header more similarly to how it is sent in the request, with a : separating the key and value, e.g. --header "User-Agent: something something".
A = or : could appear in the value, e.g. WWW-Authenticate: Basic realm="notice: private system", so either way you should use SplitN() to limit to 2 substrings (only splitting on the first separator).
mreiferson
changed the title
|
Thanks for the contribution @alwindoss, this is coming out nice so far — looks like @ploxiln's got the code review 😁 |
|
Added |
.gitignore
Outdated
| @@ -1,6 +1,8 @@ | |||
| .godeps | |||
| vendor | |||
| ./build | |||
| build/ | |||
There was a problem hiding this comment.
the line right above this should already work to ignore the build folder
There was a problem hiding this comment.
(just the build folder at the very top of the repo directory tree)
.gitignore
Outdated
| @@ -1,6 +1,8 @@ | |||
| .godeps | |||
| vendor | |||
| ./build | |||
| build/ | |||
| test | |||
There was a problem hiding this comment.
this will also ignore internal/test/ which has checked-in source code we don't want ignored
apps/nsq_to_http/nsq_to_http.go
Outdated
| key := strings.TrimSpace(sp[0]) | ||
| val := strings.TrimSpace(sp[1]) | ||
| if key == "" || val == "" { | ||
| return nil, errors.New("Not meeting the criteria") |
There was a problem hiding this comment.
both of these error strings could be more specific, e.g. fmt.Errorf("Invalid header %q", s)
Then, above, your fatal logging could print just the (first) one which is invalid. Right now it does:
log.Printf("Specified custom headers: %v", validCustomHeaders)which will be empty (maybe you meant customHeaders there)
There was a problem hiding this comment.
Moved the log.Printf("Specified custom headers: %v", validCustomHeaders) to a move appropriate place i.e., after the error check. Also, cleaned up the other logs
apps/nsq_to_http/nsq_to_http.go
Outdated
| if err != nil { | ||
| log.Fatal("--header value format should be 'key=value'") | ||
| } | ||
| log.Printf("Specified custom headers: %v", validCustomHeaders) |
There was a problem hiding this comment.
Yes it does seem redundant. But technically I need help here. For some reason I keep getting an error that I have an unused variable called validCustomHeaders
I could use a lil help here
There was a problem hiding this comment.
Ahhh, an annoying quirk of Go known as "variable shadowing". What you want on line 187 is this:
var err error
validCustomHeaders, err = parseCustomHeaders(customHeaders)
...(note the lack of :=)
There was a problem hiding this comment.
I know Go does not allow you to have unused variables but "variable shadowing" I am hearing for the first time. I have fixed it.
| var err error | ||
| validCustomHeaders, err = parseCustomHeaders(customHeaders) | ||
| if err != nil { | ||
| log.Fatal("--header value format should be 'key=value'") |
There was a problem hiding this comment.
you could include the err string in the fatal message
|
Looks good to me. Can you squash these commits down to a single commit? |
Signed-off-by: Alwin Doss <alwindoss84@gmail.com> Resolved merge conflicts Signed-off-by: Alwin Doss <alwindoss84@gmail.com> Resolved merge conflicts Signed-off-by: Alwin Doss <alwindoss84@gmail.com> Edited the description of the test case Signed-off-by: Alwin Doss <alwindoss84@gmail.com> Make the header option look similar to header option in curl Signed-off-by: Alwin Doss <alwindoss84@gmail.com> Cleaned up the logging Signed-off-by: Alwin Doss <alwindoss84@gmail.com> Removed redundant log message Signed-off-by: Alwin Doss <alwindoss84@gmail.com>
|
Squashed the commits. |
Signed-off-by: Alwin Doss alwindoss84@gmail.com