Merging ingestmanager client into generic kibana client #210

ycombinator · 2020-12-30T00:20:50Z

When the system tests feature was added to elastic-package, we created a client to communicate with the Kibana Ingest Manager API, ingestmanager.Client. Some time later, we created a more general Kibana client to communicate with the Kibana Saved Objects and Dashboard Export APIs, kibana.Client.

It makes sense to combine these two clients into one, as noted in #168 (comment). This PR attempts to do so.

elasticmachine · 2020-12-30T00:24:43Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Build Cause: Pull request #210 updated
Start Time: 2021-01-05T23:29:03.178+0000
Duration: 9 min 58 sec

Test stats 🧪

Test	Results
Failed	0
Passed	9
Skipped	0
Total	9

mtojek

Thank you for working on this. Apart from system tests, the part that can be tested only manually is export dashboards (as it uses the kibana client).

I'm worried about the failed Jenkins job:

[2020-12-30T00:43:26.104Z] Error: error running package system tests: could not complete test run: could not create test policy: could not create policy; API status code = 404; response body = {"statusCode":404,"error":"Not Found","message":"Not Found"}

I wonder if there was something missed during refactoring.

mtojek · 2021-01-04T08:39:22Z

internal/testrunner/runners/system/runner.go

@@ -180,25 +180,25 @@ func (r *runner) run() ([]testrunner.TestResult, error) {
 	}

 	// Step 2. Configure package (single data stream) via Ingest Manager APIs.
-	im, err := ingestmanager.NewClient(r.stackSettings.kibana.host, r.stackSettings.elasticsearch.username, r.stackSettings.elasticsearch.password)


I admit that I didn't analyze the source deeper, but are these Kibana/ES properties still required in stack settings.

Good catch, these are not required any more. Removed in 32fe1b2.

mtojek · 2021-01-04T08:42:57Z

internal/testrunner/runners/system/runner.go

 				Type:    ds.Type,
 				Dataset: fmt.Sprintf("%s.%s", pkg.Name, ds.Name),
 			},
 		},
 	}

 	// Add dataStream-level vars
-	dsVars := ingestmanager.Vars{}
+	dsVars := kibana.Vars{}


Thinking loud (nothing to be done immediately): The Vars structure may look to be a generic one. It may happen in the future that we need to handle another Vars structure, so some of these structs will require a relevant prefix added to their names.

mtojek · 2021-01-04T08:45:58Z

internal/kibana/policies.go

@@ -52,7 +52,7 @@ func (c *Client) CreatePolicy(p Policy) (*Policy, error) {
 func (c *Client) DeletePolicy(p Policy) error {
 	reqBody := `{ "agentPolicyId": "` + p.ID + `" }`

-	statusCode, respBody, err := c.post("agent_policies/delete", []byte(reqBody))
+	statusCode, respBody, err := c.post("/api/fleet/agent_policies/delete", []byte(reqBody))


What do you think about extracting all URL prefixes to the common place? I know it's an improvement comparing to what we have now.

Sure, but just want to clarify what you mean by URL prefixes: just the /api/fleet part (and similar prefixes from other API URLs) or something more?

Yes, I meant exactly that: '/api/fleet' or '/api/saved_objects' etc. Just to prevent missing mappings in the future (because they were hidden in multiple places in the source).

Done in 17ab10c.

mtojek

Changes look good to me. I'm worried about the CI job output that revealed a couple of unknown errors: https://beats-ci.elastic.co/blue/organizations/jenkins/Ingest-manager%2Felastic-package/detail/PR-210/6/pipeline

Would you mind looking at them?

mtojek · 2021-01-05T08:52:16Z

internal/kibana/policies.go

@@ -52,7 +52,7 @@ func (c *Client) CreatePolicy(p Policy) (*Policy, error) {
 func (c *Client) DeletePolicy(p Policy) error {
 	reqBody := `{ "agentPolicyId": "` + p.ID + `" }`

-	statusCode, respBody, err := c.post("agent_policies/delete", []byte(reqBody))
+	statusCode, respBody, err := c.post(fmt.Sprintf("%s/agent_policies/delete", FleetAPI), []byte(reqBody))


nit: Hm.. I'm wondering if it's not better to extract also the resource part (agent_policies, package_policies) as they appear in multiple places. Once it's changed, I think you can switch from fmt.Sprintf to a simple concatenation (URL prefix + operation).

I think it's more readable this way, actually. And while the prefixes are more likely to change (due to project name changes, etc.), I think the resource paths are less likely to change. So I would prefer to keep it as-is.

mtojek · 2021-01-05T08:54:46Z

internal/kibana/prefixes.go

+
+const (
+	// CoreAPI is the prefix for all Kibana Core API resources.
+	CoreAPI = "/api/kibana"


WDYT about renaming it to api.go or url_prefixes.go? Prefixes sounds a bit vague.

Done in 93223e5.

mtojek · 2021-01-05T09:06:16Z

It seems to be an unnecessary t.Log() call. I must have missed it in previous reviews:

		t.Run(test.key, func(t *testing.T) {
			err := parseElementValue(test.key, test.definition, test.value)
			if err != nil {
				t.Log(err)
			}
			if test.fail {
				require.Error(t, err)
			} else {
				require.NoError(t, err)
			}
		})

You can leave as is or cleanup the test output - up to you :) It means that the problem is:

[2021-01-05T00:31:57.673Z] internal/kibana/dashboards.go:35:10: Sprintf format %s has arg query of wrong type strings.Builder

ycombinator · 2021-01-05T23:03:47Z

It seems to be an unnecessary t.Log() call.

Removed in 7ce568b.

ycombinator mentioned this pull request Dec 30, 2020

Asset loading test runner #168

Merged

7 tasks

ycombinator requested a review from mtojek December 30, 2020 00:21

mtojek reviewed Jan 4, 2021

View reviewed changes

ycombinator requested a review from mtojek January 5, 2021 00:30

mtojek reviewed Jan 5, 2021

View reviewed changes

mtojek approved these changes Jan 5, 2021

View reviewed changes

ycombinator force-pushed the unify-kib-client branch from 7cb110e to 7ce568b Compare January 5, 2021 23:02

ycombinator added 9 commits January 5, 2021 15:28

Merging ingestmanager client into generic kibana client

51f6fb7

Refactoring: using helper get method on kibana client

fcc1be4

Formatting

a00f7f3

Fixing path construction in URLs

a23679a

Extracting API prefixes to consts

12bab3c

Removing unused field

05ea2f2

Fixing sprintf arg

875340b

Remove t.Log call

a852384

Rename URL prefix definitions file

f9e7faa

ycombinator force-pushed the unify-kib-client branch from 93223e5 to f9e7faa Compare January 5, 2021 23:28

ycombinator merged commit 5a0293c into elastic:master Jan 6, 2021

ycombinator deleted the unify-kib-client branch January 6, 2021 00:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merging ingestmanager client into generic kibana client #210

Merging ingestmanager client into generic kibana client #210

ycombinator commented Dec 30, 2020 •

edited

Loading

elasticmachine commented Dec 30, 2020 •

edited

Loading

Build stats

Test stats 🧪

mtojek left a comment

mtojek Jan 4, 2021

ycombinator Jan 5, 2021

mtojek Jan 4, 2021

mtojek Jan 4, 2021

ycombinator Jan 4, 2021

mtojek Jan 4, 2021

ycombinator Jan 5, 2021

mtojek left a comment

mtojek Jan 5, 2021

ycombinator Jan 5, 2021

mtojek Jan 5, 2021

ycombinator Jan 5, 2021

mtojek commented Jan 5, 2021

ycombinator commented Jan 5, 2021

Merging ingestmanager client into generic kibana client #210

Merging ingestmanager client into generic kibana client #210

Conversation

ycombinator commented Dec 30, 2020 • edited Loading

elasticmachine commented Dec 30, 2020 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

mtojek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtojek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtojek commented Jan 5, 2021

ycombinator commented Jan 5, 2021

ycombinator commented Dec 30, 2020 •

edited

Loading

elasticmachine commented Dec 30, 2020 •

edited

Loading