-
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
[Task Manager] Prevents Task Manager from trying to claim invalid tasks #76891
[Task Manager] Prevents Task Manager from trying to claim invalid tasks #76891
Conversation
Pinging @elastic/kibana-alerting-services (Team:Alerting Services) |
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.
The fix seems correct, but I left a question about one of the tests. Other than that, LGTM
]); | ||
}); | ||
|
||
test('it filters out invalid tasks that arent SavedObjects', async () => { |
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.
It's not immediately clear to me that these tasks aren't valid - isn't the change just to make sure the id has a task:
prefix and has a type
field? So, either the test name isn't quite right, or else there should probably be a comment explaining more. Here's the new code being called to check the validity:
kibana/src/core/server/saved_objects/serialization/serializer.ts
Lines 47 to 56 in caa4e0c
public isRawSavedObject(rawDoc: SavedObjectsRawDoc) { | |
const { type, namespace } = rawDoc._source; | |
const namespacePrefix = | |
namespace && this.registry.isSingleNamespace(type) ? `${namespace}:` : ''; | |
return Boolean( | |
type && | |
rawDoc._id.startsWith(`${namespacePrefix}${type}:`) && | |
rawDoc._source.hasOwnProperty(type) | |
); | |
} |
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.
The way I see it is that the type:
is a SavedObjects internal implementation detail.
We're using the SavedObject's validator to validate - the actual distinction seems less relevant to Task Manager as far as I can tell. 🤔
Does that make sense?
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
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Test FailuresAccessibility Tests.test/accessibility/apps/discover·ts.Discover Load a new search from the panelStandard Out
Stack Trace
Build metrics
History
To update your PR or re-run it, just comment with: |
…6891) Filters out invalid SOs from search results to prevent a never ending loop and spamming of logs in Task Manager.
* master: (65 commits) [Security Solution][Resolver] Analyzed event styling (elastic#77115) filter invalid SOs from the searc hresults in Task Manager (elastic#76891) [RUM Dashboard] Visitors by region map (elastic#77135) [Security Solution][Endpoint][Admin] Task/endpoint list actions (elastic#76555) [Ingest pipelines] Forms for processors T-U (elastic#76710) updating datatable type (elastic#77320) [ML] Fix custom URLs processing for security app (elastic#76957) [telemetry] add schema guideline + schema_check new check for --path config (elastic#75747) [ML] Transforms: API schemas and integration tests (elastic#75164) [Mappings editor] Add support for wildcard field type (elastic#76574) [Ingest Manager] Fix flyout instruction selection (elastic#77071) [Telemetry Tools] update lodash to 4.17 (elastic#77317) [APM] Service inventory redesign (elastic#76744) Hide management sections based on cluster/index privileges (elastic#67791) [Snapshot Restore] Disable steps when form is invalid (elastic#76540) [Mappings editor] Add support for positive_score_impact to rank_feature (elastic#76824) Update apm.ts (elastic#77310) [OBS] Remove beta badge, change news feed size and add external icon to news feed link (elastic#77164) [Discover] Convert legacy sort to be compatible with multi sort (elastic#76986) [APM] API Snapshot Testing (elastic#77229) ...
* master: (65 commits) [Security Solution][Resolver] Analyzed event styling (elastic#77115) filter invalid SOs from the searc hresults in Task Manager (elastic#76891) [RUM Dashboard] Visitors by region map (elastic#77135) [Security Solution][Endpoint][Admin] Task/endpoint list actions (elastic#76555) [Ingest pipelines] Forms for processors T-U (elastic#76710) updating datatable type (elastic#77320) [ML] Fix custom URLs processing for security app (elastic#76957) [telemetry] add schema guideline + schema_check new check for --path config (elastic#75747) [ML] Transforms: API schemas and integration tests (elastic#75164) [Mappings editor] Add support for wildcard field type (elastic#76574) [Ingest Manager] Fix flyout instruction selection (elastic#77071) [Telemetry Tools] update lodash to 4.17 (elastic#77317) [APM] Service inventory redesign (elastic#76744) Hide management sections based on cluster/index privileges (elastic#67791) [Snapshot Restore] Disable steps when form is invalid (elastic#76540) [Mappings editor] Add support for positive_score_impact to rank_feature (elastic#76824) Update apm.ts (elastic#77310) [OBS] Remove beta badge, change news feed size and add external icon to news feed link (elastic#77164) [Discover] Convert legacy sort to be compatible with multi sort (elastic#76986) [APM] API Snapshot Testing (elastic#77229) ...
Summary
Filters out invalid SOs from search results to prevent a never ending loop and spamming of logs in Task Manager.
closes #51222
Checklist
Delete any items that are not applicable to this PR.
For maintainers