remove angular dependencies in SavedObjectClient #20384

nreese · 2018-07-02T17:40:50Z

This PR removes all angular dependencies from the SavedObjectClient. Access to the savedObjectClient instance is provided in chrome by calling chrome.getSavedObjectsClient().

The existing SavedObjectsClientProvider is now just a wrapper around the vanilla savedObjectClient that wraps everything in an Angular Promise.

elasticmachine · 2018-07-02T18:40:27Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-07-02T19:50:30Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-07-02T20:54:03Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

nreese · 2018-07-02T21:12:10Z

jenkins, test this

elasticmachine · 2018-07-02T22:12:33Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-07-02T23:42:35Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-07-03T04:16:59Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…_objects_client

elasticmachine · 2018-07-03T18:44:34Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-07-03T20:07:48Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-07-03T21:31:22Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…_objects_client

elasticmachine · 2018-07-03T23:48:56Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

spalger

Looks good! No major feedback but I've just looked at the code so far.

spalger · 2018-07-04T00:49:41Z

src/ui/public/error_auto_create_index/error_auto_create_index.js

-    }
+export function isAutoCreateIndexError(error) {
+  return (
+    error.statusCode === 503 &&


Is error.statusCode defined here? It looks like err.res.status is being used instead in other places.

spalger · 2018-07-04T01:21:03Z

test/functional/apps/management/exports/_import_objects_with_index_pattern_conflicts.json

@@ -0,0 +1,19 @@
+[


Where is this used?

This was checked in by mistake. I will remove.

spalger · 2018-07-04T01:21:11Z

test/functional/apps/management/_import_objects.js

      await PageObjects.settings.clickKibanaSavedObjects();
      await PageObjects.settings.importFile(path.join(__dirname, 'exports', '_import_objects_saved_search.json'));
      await PageObjects.header.waitUntilLoadingHasFinished();
+      await PageObjects.settings.clickImportDone();


Are these related to this change?

not really. The test was passing locally but failing on CI. Closing the flyout allowed the test to pass locally and on CI

Hmm, any idea how that's related to this?

I have no idea. I looked into to it for the better part of Monday afternoon and and Tuesday morning. Must be timing related since it runs fine locally and only had problems in CI.

spalger · 2018-07-04T01:28:37Z

src/ui/public/saved_objects/__tests__/saved_objects_client.test.js

 import { SavedObject } from '../saved_object';

+function stubKfetch() {
+  global.kfetchStub = sinon.stub();


Not a huge fan of this, much prefer switching to jest mocks when moving from mocha but that would be even more changes to this module so I understand why you didn't do it.

That said, you could avoid using global with something like:

import { kfetch as kFetchStub } from 'ui/kfetch' jest.mock('ui/fetch', () => ({})); beforeEach(() => { // jest will trash the mock automatically no real need to cleanup in afterEach() require('ui/fetch').kfetch = sinon.stub(); });

thanks for the tip. I have been looking for a way to mock an import without global and could not think of anything. This is much better

Trying to run those changes gives. Any ideas?

Configuration error: Could not locate module ui/fetch (mapped as /Users/nathanreese/projects/kibana/src/ui/public/fetch) Please check: "moduleNameMapper": { "/^ui\/(.*)/": "/Users/nathanreese/projects/kibana/src/ui/public/$1" }, "resolver": undefined

The error was because the test was trying to mock the import of ui/fetch while the real path is ui/kfetch. Got it to work

…bal, add test to error_auto_create_index to ensure compatibility with kfetch errors

spalger

Pulled changes, created some objects, deleted some objects, used import/export, things seem to work well. Code looks good. I'm a little concerned about the functional testing change, but if you are able to explain why that fixes things I'd be more happy. Either way I think this is good to merge.

elasticmachine · 2018-07-05T17:07:41Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…_objects_client

elasticmachine · 2018-07-10T16:11:53Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

cjcenizal

Code LGTM! I had one question. You might also want to rename the processBatchQueue and createSavedObject methods in saved_objects_client.js to _processBatchQueue and _createSavedObject which would make them consistent with how other private methods are named. It doesn't look like they're used externally.

cjcenizal · 2018-07-11T18:17:48Z

src/ui/public/error_auto_create_index/error_auto_create_index.js

-    }
+export function isAutoCreateIndexError(error) {
+  return (
+    get(error, 'res.status') === 503 &&


Was the original code incorrect, in which we were comparing error.statusCode?

The original code used the error produced in saved_object_client _request method. But kfetch creates its own error type. Just needed to update to handle the error thrown by kfetch.

…_objects_client

elasticmachine · 2018-07-11T20:43:10Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* create saved object client that is native javascript * fix savedObjectClient unit tests * get saved object client from chrome when being used outside of angular * update error handlers to pull status code from FetchError * add some debug messages to failing functional test * revert changes to management/_objects * add clicks to import done in import objects test * take screenshots of test only failing in CI * remove functional test screenshot code since test is passing in CI now * remove unused file, clean up saved_objects_client test to not use global, add test to error_auto_create_index to ensure compatibility with kfetch errors * add body to kfetch error * update savedObjectClient.bulkCreate * add bulkCreate wrapper to SavedObjectsClientProvider * mark _createSavedObject and _processBatchQueue as private methods

sorenlouv · 2018-08-19T21:47:13Z

src/ui/public/error_auto_create_index/error_auto_create_index.test.js

+    afterEach(() => fetchMock.restore());
+
+    test('should return false', async () => {
+      let gotError = false;


@nreese stumbled over this while updating kfetch. fyi expect.assertions(1) is very handy in this exact scenario (https://jest-bot.github.io/jest/docs/expect.html#expectassertionsnumber)

nreese changed the title ~~remove angular dependencies on SavedObjectClient~~ remove angular dependencies in SavedObjectClient Jul 2, 2018

nreese added chore Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// v7.0.0 v6.4.0 labels Jul 2, 2018

nreese added 3 commits July 2, 2018 12:50

create saved object client that is native javascript

57ef8f2

fix savedObjectClient unit tests

062bf3e

get saved object client from chrome when being used outside of angular

d086830

nreese force-pushed the saved_objects_client branch from c087fe1 to d086830 Compare July 2, 2018 18:51

update error handlers to pull status code from FetchError

56f92bb

add some debug messages to failing functional test

bda978b

revert changes to management/_objects

1ebfc8a

Merge branch 'master' of https://github.com/elastic/kibana into saved…

f1629f4

…_objects_client

add clicks to import done in import objects test

ce30939

take screenshots of test only failing in CI

dc8509f

nreese added 2 commits July 3, 2018 16:36

Merge branch 'master' of https://github.com/elastic/kibana into saved…

68a62bc

…_objects_client

remove functional test screenshot code since test is passing in CI now

773a0cd

nreese requested review from rhoboat and spalger July 3, 2018 23:52

spalger suggested changes Jul 4, 2018

View reviewed changes

nreese added 2 commits July 5, 2018 09:47

remove unused file, clean up saved_objects_client test to not use glo…

b87a31a

…bal, add test to error_auto_create_index to ensure compatibility with kfetch errors

add body to kfetch error

bcce492

spalger approved these changes Jul 5, 2018

View reviewed changes

nreese added 3 commits July 10, 2018 08:29

Merge branch 'master' of https://github.com/elastic/kibana into saved…

0eedca8

…_objects_client

update savedObjectClient.bulkCreate

79dbabe

add bulkCreate wrapper to SavedObjectsClientProvider

e295c79

nreese requested review from cjcenizal and removed request for rhoboat July 11, 2018 14:12

cjcenizal approved these changes Jul 11, 2018

View reviewed changes

nreese added 2 commits July 11, 2018 13:21

Merge branch 'master' of https://github.com/elastic/kibana into saved…

1801eb0

…_objects_client

mark _createSavedObject and _processBatchQueue as private methods

a8f5a1f

nreese merged commit 572f8bb into elastic:master Jul 11, 2018

nreese mentioned this pull request Jul 11, 2018

[6.x] remove angular dependencies in SavedObjectClient (#20384) #20693

Merged

nreese mentioned this pull request Aug 1, 2018

use SavedObjectsClientProvider to provide SavedObjectLoader with an angular wrapped saved object client #21541

Merged

jen-huang mentioned this pull request Aug 13, 2018

Fix saving index pattern when there is a conflict #21947

Merged

sorenlouv reviewed Aug 19, 2018

View reviewed changes

remove angular dependencies in SavedObjectClient #20384

remove angular dependencies in SavedObjectClient #20384

Uh oh!

Conversation

nreese commented Jul 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Jul 2, 2018

💔 Build Failed

Uh oh!

elasticmachine commented Jul 2, 2018

💔 Build Failed

Uh oh!

elasticmachine commented Jul 2, 2018

💔 Build Failed

Uh oh!

nreese commented Jul 2, 2018

Uh oh!

elasticmachine commented Jul 2, 2018

💔 Build Failed

Uh oh!

elasticmachine commented Jul 2, 2018

💔 Build Failed

Uh oh!

elasticmachine commented Jul 3, 2018

💔 Build Failed

Uh oh!

elasticmachine commented Jul 3, 2018

💔 Build Failed

Uh oh!

elasticmachine commented Jul 3, 2018

💔 Build Failed

Uh oh!

elasticmachine commented Jul 3, 2018

💔 Build Failed

Uh oh!

elasticmachine commented Jul 3, 2018

💚 Build Succeeded

Uh oh!

spalger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spalger Jul 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spalger left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jul 5, 2018

💚 Build Succeeded

Uh oh!

elasticmachine commented Jul 10, 2018

💚 Build Succeeded

Uh oh!

cjcenizal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nreese commented Jul 2, 2018 •

edited

Loading

spalger Jul 4, 2018 •

edited

Loading

nreese Jul 11, 2018 •

edited

Loading

sorenlouv Aug 19, 2018 •

edited

Loading