Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

!signed-cla

Thanks for the PR!

I don't think this change is unreasonable. Hipska has a good point about handling the error messages, so we need to pay attention to that. Probably the best way to decide if we should parse it as a file or a url is to look for a character sequence that clearly identifies it. Parsing files as urls will still give you a url, which is what caused the original problem. This PR does in fact resolve that problem, it just comes at the cost of checking the disk for matching files first.. Which honestly isn't really a big deal.

If we wanted to stick closely to the intent of the user configuring the plugin, we could match against the regex ^\w+:// and if it matches use url parsing, and if it doesn't, use file parsing. This will remove all ambiguity, and allow for clear error messages that say things like "couldn't read the file %q" or "couldn't parse the url %q".

@ssoroka well my fear is that you, with the change applied, cannot specify a URL if a file with that name exists. Let's make an example. Say a) you want to access a file named http://myfile and b) you want to access a server at http://myserver and let's assume you have (for whatever reason) the files http://myfile and http://myserver on-disk...

With the current implementation you can achieve this by specifying
for a) file://'http://myfile'
for b) http://myserver
and it fill access the file for a) and the server for b).

With the change you specify
for a) http://myfile
for b) http://myserver
while correct for a) it now will access the file http://myserver for b) instead of the server.

Sorry @akrantz01 if my comment was too harsh, but I think this kind of implicit behavior is cumbersome and might lead to unintended effects.

Hmmm and after reading the code again I see that my example is wrong. :-) So currently specifying file://'http://myfile will try to read 'file://http://myfile' instead of 'http://myfile'... So I see your point now @akrantz01...

The team probably should discuss this as @Hipska said, but my preference would be to implement the handling of the file:// scheme such as

func loadConfig(config string) ([]byte, error) {
	if strings.HasPrefix(config, "file://") {
		return ioutil.ReadFile(strings.TrimPrefix(config, "file://"))
	}

	u, err := url.Parse(config)
	if err != nil {
		return nil, err
	}

	switch u.Scheme {
	case "https", "http":
		return fetchConfig(u)
	}

	// If it isn't known scheme, try it as a file.
	return ioutil.ReadFile(config)
}

Isn't fetchConfig also able to handle file:// urls? So case scheme is 'file' then do something ;)

To the best of my knowledge, I don't think you can create files with forward slashes in the name on any platform, so I think it'd be best to go with @ssoroka's suggestion. It wouldn't require any changes from the user and would still handle actual URLs properly.

Regarding @Hipska's point, we could still add a case for file:// URLs so users could be explicit if they wanted.

Actually, if we wanted to support file:// URLs through fetchConfig, it would take a bit more work since the default HTTP client doesn't support the file scheme.

We will get problems with parameters like file://https:test. Check out https://goplay.space/#SqDP6ybX7Af or
the attached code. That's why I tried to handle the file:// part first. :-)

@ssoroka and I decided that it would be simplest just to use the regex before parsing the URL, since then the implementation isn't dependent on the internals of url.Parse if they ever change. It was also decided to not support file://, since it isn't used anywhere else.