feat: remove trailing slash from http request if it exists in url

[yquansah]

This PR will remove the trailing slash from the URL of the http request if it exists.

Completes FLI-339

[yquansah]

I am very undecided how I wanted to architect the integration tests with this PR. Suggestions would be appreciated 🤔

I threaded through the fliptAddr and constructed my own http client for this purpose.

[codecov]

Codecov Report

Merging #1754 (8d171ea) into main (029960b) will decrease coverage by 8.05%.
The diff coverage is 66.66%.

❗ Current head 8d171ea differs from pull request most recent head f61b92a. Consider uploading reports for the commit f61b92a to get more accurate results

@@            Coverage Diff             @@
##             main    #1754      +/-   ##
==========================================
- Coverage   77.96%   69.91%   -8.05%     
==========================================
  Files          55       58       +3     
  Lines        4905     5478     +573     
==========================================
+ Hits         3824     3830       +6     
- Misses        857     1423     +566     
- Partials      224      225       +1     

Impacted Files	Coverage Δ
internal/cmd/http.go	3.70% <66.66%> (ø)

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[GeorgeMac]

Yeah I think we should consider not putting these in the API tests.
They seem like an awkward place to shoe-horn them in.

I think perhaps we're reaching the need for a general light refactoring of test layouts here.
But maybe not just for this issue. I will ruminate on that a bit.

For now though, maybe this is something we can just unit test? My thinking here is: what is most important about this issue is that all of our clients work. Not that we do or do not support trailing slashes. We need a single working path for all our RPCs at a minimum.

Currently, all our clients work without this feature. Adding it is a nice to have to be more flexible for future clients. When it comes to integration tests that are valuable here: I wonder if what is more impotant as an integration test, is to run our clients in different languages against Flipt. Instead of trying to fabricate this implementation detail in an integration test, we want an easy way to slot in a new client and ensure it works against a running Flipt.

What do you think too @markphelps ?

(Aside: maybe these integration tests should be renamed compatability tests)

[GeorgeMac]

strings.TrimSuffix actually already calls strings.HasSuffix.
I think you can boil this all down to just the following (avoids panic too):

Suggested change

	if strings.HasSuffix(r.URL.Path, "/") {
	// Panic if URL can not be parsed if a trailing slash is trimmed.
	nurl, err := url.Parse(strings.TrimSuffix(r.URL.String(), "/"))
	if err != nil {
	panic(err)
	}
	r.URL = nurl
	}
	r.URL.Path = strings.TrimSuffix(r.URL.Path)

[yquansah]

@GeorgeMac Thank you George!

[markphelps]

Yeah I think we should consider not putting these in the API tests. They seem like an awkward place to shoe-horn them in.

I think perhaps we're reaching the need for a general light refactoring of test layouts here. But maybe not just for this issue. I will ruminate on that a bit.

For now though, maybe this is something we can just unit test? My thinking here is: what is most important about this issue is that all of our clients work. Not that we do or do not support trailing slashes. We need a single working path for all our RPCs at a minimum.

Currently, all our clients work without this feature. Adding it is a nice to have to be more flexible for future clients. When it comes to integration tests that are valuable here: I wonder if what is more impotant as an integration test, is to run our clients in different languages against Flipt. Instead of trying to fabricate this implementation detail in an integration test, we want an easy way to slot in a new client and ensure it works against a running Flipt.

What do you think too @markphelps ?

(Aside: maybe these integration tests should be renamed compatability tests)

Agreed about running clients in other languages for compatibility testing, adding this as an IT prob doesn't fit within out existing IT stack