provide -admin-access-log-path #5858
Conversation
command/connect/envoy/envoy.go
Outdated
| @@ -258,13 +263,19 @@ func (c *cmd) templateArgs() (*BootstrapTplArgs, error) { | |||
| cluster = c.sidecarFor | |||
| } | |||
|
|
|||
| adminAccessLogPath := c.adminAccessLogPath | |||
| if len(adminAccessLogPath) == 0 { | |||
There was a problem hiding this comment.
Would this be a little clearer with if adminAccessLogPath == ""? Since it's a string.
There was a problem hiding this comment.
I always wonder about the same thing and I read the first answer of https://stackoverflow.com/questions/18594330/what-is-the-best-way-to-test-for-an-empty-string-in-go. If you find == "" better, I would be happy to change it. I don't have an opinion on this one.
There was a problem hiding this comment.
I think == "" would be clearer and more consistent. I did a search through the Consul repo and found that len(x) == 0 didn't seem to ever be used with strings.
Thanks!
You can provide that flag on the commandline and it will show the admin access logs on |
Just for the record, using the exec feature of |
There was a problem hiding this comment.
I think we should be more explicit about the default being /dev/null in the comments and usage text rather than suggesting that is an option for the user.
I also think it would be consistent with other fields to actually apply that default even if the struct is empty in the handling code (like we do with agent address etc) but it isn't really a big deal either way and fine as it is!
Approving as this is fine to merge but the usage comment update would be nice IMO!
| @@ -27,6 +27,11 @@ type BootstrapTplArgs struct { | |||
| // TLS is enabled. | |||
| AgentCAFile string | |||
|
|
|||
| // AdminAccessLogPath The path to write the access log for the | |||
| // administration server. If no access log is desired specify | |||
| // "/dev/null". | |||
There was a problem hiding this comment.
Should we just make /dev/null be the default so specifying empty is also valid? This is kinda internal only but we do a bunch of default setting before the template already so this seems like a reasonable approach here too.
There was a problem hiding this comment.
I made it so that it can also be left empty and then the default would be picked up.
|
@banks I changed the comments and flag description as well as my comment above re how to run envoy. 👍 |
Fixes #5231.