-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Endpoint] EMT-65: make endpoint data types common, restructure #54772
Changes from all commits
6577ba0
5127ff5
1128056
ea26d3d
d96e1f9
49d745c
f5630f2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
export class EndpointAppConstants { | ||
static ENDPOINT_INDEX_NAME = 'endpoint-agent*'; | ||
} | ||
|
||
export interface EndpointResultList { | ||
// the endpoint restricted by the page size | ||
endpoints: EndpointMetadata[]; | ||
// the total number of unique endpoints in the index | ||
total: number; | ||
// the page size requested | ||
request_page_size: number; | ||
// the index requested | ||
request_page_index: number; | ||
} | ||
|
||
export interface EndpointMetadata { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My last concern, @peluja1012 has a PR where types are being handled a bit differently: https://github.com/elastic/kibana/pull/55800/files#diff-a19f3a4b9567cb1b06760465a6a74d43R25 That PR is using Thoughts @paul-tavares @peluja1012 @nnamdifrankie @oatkiller There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kevinlog I could be wrong, but I think that when you define a type directly as an object, the (ESLint) formater automatically converts it to an Interface. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kevinlog I do not think we should be mixing this two. I prefer the interface and it is advised not to mix both. I think type is good for shorthand or derived type definition. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. got it. still though, we discussed getting everything across the app into a common types file until we need to break things out. Would it still make sense to follow the same pattern for defining an endpoint metadata response and an alert response? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kevinlog like Paul said, the linter will automatically convert types to interfaces when possible, so you probably don't need to think about it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @paul-tavares @oatkiller thanks for the knowledge and responses, lgtm then |
||
event: { | ||
created: Date; | ||
}; | ||
endpoint: { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think |
||
policy: { | ||
id: string; | ||
}; | ||
}; | ||
agent: { | ||
version: string; | ||
id: string; | ||
}; | ||
host: { | ||
id: string; | ||
hostname: string; | ||
ip: string[]; | ||
mac: string[]; | ||
os: { | ||
name: string; | ||
full: string; | ||
version: string; | ||
}; | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean to move a
Class
to thistypes.ts
file? I don't see the FE needing itThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nnamdifrankie see my prior comment here. I don't think you should have a Class in this file (maybe you meant
interface
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @nnamdifrankie is defining constants here, not types necessarily. So I would say that maybe we could move this somewhere else, if we're trying to have this file contain and export types for the app.