search

gliderlabs / logspout

Log routing for Docker container logs
MIT License
4.66k stars 680 forks source link

Keep track of path when parsing route URL #492

Open claustres opened 4 years ago

claustres commented 4 years ago

When parsing the route URL here the path is not kept while it could be useful for some adapters.

For instance, we've developed https://github.com/kalisio/logspout-slack and we need to provide the path part of the webhook ( of the form https://hooks.slack.com/services/xxx) as a configuration option.

michaelshobbs commented 4 years ago

i'd definitely accept a PR addressing this

michaelshobbs commented 4 years ago

alternatively, could you use Route.Options to pass the path around?

claustres commented 4 years ago

If it seems useful to the community I can try. However, I'm really new to Go and it could take time before I write decent code ;-)

claustres commented 4 years ago

@michaelshobbs Do you mean add the path as a query parameter and get it back as an option in the route?

michaelshobbs commented 4 years ago

Apologies it's been awhile since I've been in this codebase. I assumed you already had a way to get the path into the system and we were just dropping it. Is that true?

claustres commented 4 years ago

I am not sure to get you but what I would like to be able to do is for instance:

docker run --name="logspout" \
    --volume=/var/run/docker.sock:/var/run/docker.sock \
    ...
    logspout:v3.2.11 \
    slack://hooks.slack.com/services/xxx

But in my adapter when I'd like to get back the route info I can't get the path back:

func NewSlackAdapter(route *router.Route) (router.LogAdapter, error) {
    // Here route.Address is "hooks.slack.com", the path part "/services/xxx" has been lost
        ...
}

Maybe I am wrong but it appears to me neither the type to the parsing take care of it.

Maybe as you suggest there is another way to get the path back as well.

michaelshobbs commented 4 years ago

Maybe I am wrong but it appears to me neither the type to the parsing take care of it.

that is correct. however it does exist in the variable u (type URL) defined on L91

https://github.com/gliderlabs/logspout/blob/4e88b84bf163775801b935e0afc9396c6c0dbed4/router/routes.go#L91

golang net/url docs: https://golang.org/pkg/net/url/#URL

So we could store u.Path in Route.Options OR we could add another field to the type. I'm fine with either.

claustres commented 4 years ago

I think that adding a new field is more in line with the existing base.