-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Fleet] parse elasticsearch.source_mode
from manifest + small refactor
#144464
Conversation
Pinging @elastic/fleet (Team:Fleet) |
parsedElasticsearchEntry.privileges = elasticsearch.privileges; | ||
} | ||
|
||
if (elasticsearch?.source_mode) { |
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.
Source mode can only be one of two values, not sure if we should be validating here? We only seem to do high level validation
@@ -351,6 +351,7 @@ export interface RegistryElasticsearch { | |||
'index_template.settings'?: estypes.IndicesIndexSettings; | |||
'index_template.mappings'?: estypes.MappingTypeMapping; | |||
'ingest_pipeline.name'?: 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.
I am curious about the usage of this key, it looks like to me that is not used anywhere
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.
@nchaulet do you mean ingest_pipeline.name
? It was added as part of this PR to make our logic the same as registry info:
https://github.com/elastic/kibana/pull/126915/files
It was added to the registry 2 years ago here elastic/package-registry#564 , I am not sure it was ever implemented in Kibana (until we implemented it in the above PR to make sure we match the registry) but I am nervous to delete it :D Should I just go for it?
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.
@nchaulet turns out the system package uses this functionality
a59d59c
to
c0c6c7a
Compare
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
Unknown metric groupsAPI count
ESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @hop-dev |
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.
LGTM 🚀
Summary
Part of #141211.
Add
source_mode
to the elasticsearch fields we parse out of the package manifest. I noticed an opportunity to break out some small unit tests while I was in the code as well.