muesli / beehive

A flexible event/agent & automation system with lots of bees 🐝
GNU Affero General Public License v3.0
6.32k stars 324 forks source link

MS Windows fixes #310

Open rubiojr opened 4 years ago

rubiojr commented 4 years ago

For some extra context, Go URL parsing in Windows can be a bit surprising.

Parsing file:///c:/beehive.conf, the URI format documented here, will set the URL path to /c:/beehive.conf. This behaviour is captured in https://github.com/golang/go/issues/6027.

If we use a URL like file://c:/beehive.conf, the drive name will be parsed as the host and the path will miss the drive name.

package main

import (
        "fmt"
        "net/url"
)

func main() {
        u, _ := url.Parse(`file://c:/beehive.conf`)
        fmt.Println(u.Host)
        fmt.Println(u.Path)
}

This prints:

c:
/beehive.conf

We workaround it patching the URL path and host so URLs like crypt://sercret@c:/beehive.conf behave as expected.

rubiojr commented 4 years ago

I think I can come up with a slightly cleaner fix so I don't have to patch every single backend method.

rubiojr commented 4 years ago

@muesli if you don't entirely dislike this patch, we could merge as is to fix the windows situation and I'll work on a new PR, to clean things up a bit.

rubiojr commented 4 years ago

This is looking good now.

Ended up wrapping net/url URL with a drop-in replacement that helps to encapsulate our platform specific logic to patch URLs in Windows. The wrapper is heavily exercised buy our current tests so I didn't consider necessary to add dedicated unit tests for it.

Also took the chance to add some test helpers and clean things up a bit.

rubiojr commented 4 years ago

This is looking good now.

Not yet, just found one more windows niggle when executing the binary.

rubiojr commented 4 years ago

Fixed in https://github.com/muesli/beehive/pull/310/commits/602668adc400c0a5ef2465d1fe1080ef1099e060

muesli commented 4 years ago

@rubiojr Just a heads up, @penguwin and I have been pondering how to handle the config URL situation properly for a while and have come up with a few fixes. I think we'll probably have to combine our approaches.

rubiojr commented 4 years ago

sounds good!

rubiojr commented 4 years ago

Awesome @penguwin, let me have a look at the implementation there and I'll get back to you ASAP.

penguwin commented 4 years ago

cool, if there's anything I can help/assist you with let me know

rubiojr commented 4 years ago

@penguwin: rebased the branch but not without some :sweat_smile:, since I forgot what I did here in May :smile: .

It'll need some extra polish before asking for a reviewing it again, so I'm taking care of that first.

If y'all haven't started it, I'll also be looking into extracting the configuration system into it's own repo, see if we can make that work for both beehive and knoxite, to avoid duplicated work and the tedious maintenance task of cherry picking fixes for it from one project to the other.