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.

Sign up for GitHub

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

Jump to bottom

EMT-287: update schema with elastic agent id #62252

Merged
merged 1 commit into from
Apr 2, 2020
Merged

EMT-287: update schema with elastic agent id #62252

merged 1 commit into from
Apr 2, 2020

Conversation

nnamdifrankie
Copy link
Contributor

@nnamdifrankie nnamdifrankie commented Apr 2, 2020 •
edited
Loading

Summary

https://github.com/elastic/endpoint-app-team/issues/287
Update data structures and mapping to add elastic agent id to the metadata document. Updated the tests.

Checklist

@nnamdifrankie nnamdifrankie requested a review from a team as a code owner April 2, 2020 00:56
@nnamdifrankie nnamdifrankie added v7.8.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Apr 2, 2020
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

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

paul-tavares
paul-tavares approved these changes Apr 2, 2020
View reviewed changes
Copy link
Contributor

@paul-tavares paul-tavares left a comment

Choose a reason for hiding this comment

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

LGTM 👍

x-pack/plugins/endpoint/common/generate_data.ts
@@ -83,6 +83,11 @@ const OTHER_EVENT_CATEGORIES: EventInfo[] = [
];

interface HostInfo {
elastic: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Outside of the scope of this PR, but wanted to mention it:
It feels like the types/interfaces defined here and duplicated in types.ts should be combined so that they come from one single source.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Outside of the scope of this PR, but wanted to mention it:
It feels like the types/interfaces defined here and duplicated in types.ts should be combined so that they come from one single source.

Well, some of these code was changed without my notice. In my view they are two different application that knows about each other and the use cases are different. For now it is fine to leave it separate but makes for some maintenance work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah this type can be refactored with the types in types.ts soon, doesn't need to be in this PR though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see the challenge now @marshallmain 😄. Unrelated to this PR, I just added a new generator method to the EndpointDocGenerator and attempted to reference a method/type from endpoint/public/ from this file and got an ESLint error. Some of our stuff inside of the /public/ folder may need to first be moved to this top-level location.

x-pack/test/functional/es_archives/endpoint/metadata/api_feature/data.json
@@ -10,6 +10,11 @@
"version": "6.6.1",
"name" : "Elastic Endpoint"
},
"elastic": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: did you update this es_archive manually? or did you use some sort of a tool that just add "stuff" to it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Question: did you update this es_archive manually? or did you use some sort of a tool that just add "stuff" to it?

Yes we have to make incremental changes to the data because the changes are relative, e.g. in this case all events for a particular endpoint should have the same elastic.agent.id. And we have to be backward compatible with other tests too.

Copy link
Contributor

Choose a reason for hiding this comment

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

So I assume you updated the file manually?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I updated manually.

@paul-tavares
Copy link
Contributor

One thing that I just remembered - does this impact the indexing changes/updates that @jonathan-buttner is doing to the endpoint package over at epm?

charlie-pichette
charlie-pichette approved these changes Apr 2, 2020
View reviewed changes
Copy link
Contributor

@charlie-pichette charlie-pichette left a comment

Choose a reason for hiding this comment

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

Looks good. 👍

@jonathan-buttner
Copy link
Contributor

One thing that I just remembered - does this impact the indexing changes/updates that @jonathan-buttner is doing to the endpoint package over at epm?

Thanks @paul-tavares, yeah I made the changes in the endpoint package already 👍

@nnamdifrankie nnamdifrankie merged commit cf81ef6 into elastic:master Apr 2, 2020
@nnamdifrankie nnamdifrankie deleted the EMT-287_add_elastic_agent_id branch April 2, 2020 15:21
gmmorris added a commit to gmmorris/kibana that referenced this pull request Apr 2, 2020
@gmmorris
Merge branch 'master' into event-log/query-support
3eada98 
* master:
  Switch to embeddable factory interface with optional override (elastic#61165)
  fix text error in diagrams (elastic#62101)
  [Index management] Prepare support Index template V2 format (elastic#61588)
  Updates dashboard images (elastic#62011)
  [Maps] remove MapBounds type (elastic#62332)
  [Uptime] Convert anomaly Jobs name to lowercase to comply with… (elastic#62293)
  Make d3 place nicely with object values (elastic#62004)
  EMT-287: update schema with elastic agent id (elastic#62252)
  [Maps] fix replaceLayerList to handle case where map is not intialized (elastic#62202)
  Remove support for deprecated xpack.telemetry configurations (elastic#51142)
  [Uptime] Remove static constant for index name completely (elastic#62256)
  [APM] E2E: install dependencies for vanilla workspaces (elastic#62178)
  [backport] Bump to 5.1.3 (elastic#62286)
  Show server name in Remote Cluster detail panel (elastic#62250)
  Rename some alert types (elastic#61693)
  changing duration type to ms, s, m (elastic#62265)
  [ML] Clear Kibana index pattern cache on creation or form reset. (elastic#62184)
  Move `src/legacy/server/index_patterns` to data plugin (server) (Remove step) (elastic#61618)
  [NP] Remove IndexedArray usage in Schemas (elastic#61410)
@nnamdifrankie nnamdifrankie mentioned this pull request Apr 2, 2020
Merged
nnamdifrankie added a commit that referenced this pull request Apr 3, 2020
@nnamdifrankie
[Endpoint] EMT-287: update schema with elastic agent id (#62252) (#62361
03f8c95 
)

[Endpoint] EMT-287: update schema with elastic agent id
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paul-tavares paul-tavares paul-tavares approved these changes

@marshallmain marshallmain marshallmain approved these changes

@jonathan-buttner jonathan-buttner jonathan-buttner approved these changes

@kevinlog kevinlog kevinlog approved these changes

@charlie-pichette charlie-pichette charlie-pichette approved these changes

Assignees
No one assigned
Labels
release_note:skip Skip the PR/issue when compiling release notes v7.8.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@nnamdifrankie @kibanamachine @paul-tavares @jonathan-buttner @marshallmain @kevinlog @charlie-pichette