Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EMT-661: use new metadata current #74394

Merged

Conversation

nnamdifrankie
Copy link
Contributor

@nnamdifrankie nnamdifrankie commented Aug 5, 2020

Summary

https://github.com/elastic/endpoint-app-team/issues/661
#75153
https://github.com/elastic/security-team/issues/72

  • refactor to use new metadata current.
  • update test and queries.

Checklist

Delete any items that are not applicable to this PR.

@nnamdifrankie nnamdifrankie added v7.10.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Aug 5, 2020
@nnamdifrankie
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

merge conflict between base and head

@nnamdifrankie nnamdifrankie marked this pull request as ready for review August 11, 2020 01:22
@nnamdifrankie nnamdifrankie requested review from a team as code owners August 11, 2020 01:22

import { Client } from '@elastic/elasticsearch';

export async function putTransform(getService: (serviceName: 'es') => Client, transformId: string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we put this transform in to get the tests to pass, which is good. However, I pulled this down and tried it out and the UI breaks without this transform.

Can we put the transform in in the interim before it's available via the package? I don't want to UI to break indefinitely before there's a package available.

Let me know if I'm missing something.

Copy link
Contributor

@kevinlog kevinlog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good for getting things set up for the transform and new index, however I want to wait on merging this in until it's in a state that it won't break the UI.

Is it possible to add the transform in another way until it's possible to ship it with the package? Similar to the way it's added in the tests?

Let me know if I'm missing anything

@nnamdifrankie nnamdifrankie marked this pull request as draft August 12, 2020 19:17
# Conflicts:
#	x-pack/test/security_solution_endpoint/apps/endpoint/endpoint_list.ts
@nnamdifrankie
Copy link
Contributor Author

@elasticmachine merge upstream

@nnamdifrankie
Copy link
Contributor Author

@elasticmachine merge upstream

1 similar comment
@nnamdifrankie
Copy link
Contributor Author

@elasticmachine merge upstream

@nnamdifrankie
Copy link
Contributor Author

@elasticmachine test this

@cvolzke4
Copy link

cvolzke4 commented Sep 7, 2020

Hi Neptunium, do you have a minute to review my bug fix?
neptunian/react-photo-gallery#184

@nnamdifrankie
Copy link
Contributor Author

@elasticmachine merge upstream

@@ -79,7 +79,7 @@ export default ({ getPageObjects, getService }: FtrProviderContext) => {
await testSubjects.exists('emptyPolicyTable');
});

it('finds data after load and polling', async () => {
it.skip('finds data after load and polling', async () => {
Copy link
Contributor

@kevinlog kevinlog Sep 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can rework this test. For polling, it would probably make sense to load data directly in the destination index to ensure that we update the UI correctly. We could break it out to another, isolated integration test where we can load data to the destination index to test this particular case. Alternatively, we get the transform to run more quickly for the test, but that sounds non-trivial.

We can try this out in another PR IMO.

FYI @paul-tavares let me know your thoughts

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kevinlog To cut down cost I suggest that we move this to when data is loaded. I believe you were trying to test the timer refresh?

@neptunian
Copy link
Contributor

Thanks for doing this! Installing the transforms looks good and happy with the changes made. Tested and works.

@kevinlog
Copy link
Contributor

kevinlog commented Sep 8, 2020

Checked it out and it works with UI

Real endpoint shows up, transform gets created as expected
image

Destination index getting docs:
image

Loaded in 100 more, Endpoints showed up with transform running:
image

🎉 🚢

@nnamdifrankie nnamdifrankie merged commit 2ed4b57 into elastic:master Sep 8, 2020
@nnamdifrankie nnamdifrankie deleted the EMT-661-use-metadata-current-index branch September 8, 2020 21:56
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

async chunks size

id value diff baseline
ingestManager 1.1MB +22.0B 1.1MB

page load bundle size

id value diff baseline
ingestManager 468.4KB +48.0B 468.4KB

distributable file count

id value diff baseline
default 45458 +3 45455

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

nnamdifrankie added a commit to nnamdifrankie/kibana that referenced this pull request Sep 9, 2020
… installation (elastic#74394)

[SecuritySolution-Ingest]: use new metadata current and add transform installation
nnamdifrankie added a commit that referenced this pull request Sep 9, 2020
… installation (#74394) (#77065)

[SecuritySolution-Ingest]: use new metadata current and add transform installation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants