feat: grail filter segments client

[warber]

What this PR does / Why we need it:

This PR introduces the http client for grail filter segments to be used by monaco and terraform.

Special notes for your reviewer:

Does this PR introduce a user-facing change?

[github-actions]

Test Results

  3 files  ± 0   54 suites  +6   32s ⏱️ -1s
268 tests +16  268 ✅ +16  0 💤 ±0  0 ❌ ±0 
804 runs  +48  804 ✅ +48  0 💤 ±0  0 ❌ ±0 

Results for commit 1d2457e. ± Comparison against base commit 4a7af80.

♻️ This comment has been updated with latest results.

[Laubi]

These differ from the other fields, why is that? Is it not necessary?

[jskelin]

By default, not all parts of the configuration are included. If a user doesn't specify what exactly it needs, we assume that it requests everything.

[Laubi]

The question is why the ExteralId is missing here, as the list queries all by default.

[Laubi]

To the points mentioned above, please also add documentation to all public methods, with all the assumptions and special cases we're taking

[Laubi]

Major points that are still required:

1. 0% test coverage of the filtersegments. I gave an example in my comments.
   a) This is because of the mocks. We should do fakes instead of mocks and use a local server and respond the HTTP results directly. Then, the inner code would be tested.
   b) Because of that, a real bug was not detected that should have been easily detectable, as it's not even in the method doc.

2. Inconsistent error return values. Sometimes you return a plain err, sometimes you wrap it. Please consistently wrap it.

3. The inner test names are not consistent in clients/grailfiltersegments/filtersegment_test.go

[Laubi]

Suggested change

	if ro.QueryParams == nil && ro.QueryParams.Has("add-fields") {
	if ro.QueryParams == nil && !ro.QueryParams.Has("add-fields") {

When I saw this, I noticed that there are no unit tests at all present. Please add some.
For this small check, I came up with this test:

func TestGet_AllFieldsArePresentIfNotSpecified(t *testing.T) {
	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		t.Log(r.URL.String())

		fields := r.URL.Query()["add-fields"]
		require.ElementsMatch(t, []string{"INCLUDES", "VARIABLES", "EXTERNALID", "RESOURCECONTEXT"}, fields)
	}))
	defer server.Close()
	u, err := url.Parse(server.URL)
	require.NoError(t, err)

	c := NewClient(rest.NewClient(u, server.Client()))

	resp, err := c.Get(context.TODO(), "id", rest.RequestOptions{})
	require.NoError(t, err)

	require.Equal(t, http.StatusOK, resp.StatusCode)
}

Please add some basic tests for all endpoints, e.g. input validation

[Laubi]

One more thing I noticed: Sometimes (like here) you are directly returning from the client, while in other cases (like Create) you are checking err and add context to it. I'd suggest doing this everywhere, so here and in Update adding fetching all items failed or updating failed below

[Laubi]

The question is why the ExteralId is missing here, as the list queries all by default.

[Laubi]

Here you're doing something similar to what I commented before: Sometimes you do add information to the errors, sometimes you don't (e.g. L124 you add information). I suggest adding everywhere the context what's going wrong. Please have a look where you only return the err without extra information and add some.

[Laubi]

Similar to the other file, the prefix is not needed. Here it's even wrong

Suggested change

	t.Run("Create - Create OK", func(t *testing.T) {
	t.Run("Creates if config does not exist", func(t *testing.T) {

[Laubi]

This file has 0% test coverage

[Laubi]

Suggested change

	errors.As(err, &apiErr)
	`assert.ErrorAs(t, err, &apiErr)`

Nit: This will check that the error is really of that type. Otherwise, you are currently silently swallowing the error. In case the As would fail, the Equals after will fail, but we would not know why/what error type it really was