-
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
[ResponseOps][TaskManager] TaskTypeDictionary::get()
no longer throw
s
#189591
Conversation
…ow`s resolves elastic#189560 Removes the `throw` from `TaskTypeDictionary::get()` and have it return `TaskDefinition | undefined` instead. Then fix all the places that breaks. This could probably never happen when a task is running - the definition would be needed for a bunch of things prior to this (like polling for the task). However, it has occurred during metric generation, and likely more code in the future will use this, so the new API has less of a "suprise! Error!" aspect to it, for such an innocent looking method name. Several places in the existing code did not have to be changed as they already guards for checking null-ish returns from the method :-)
Pinging @elastic/response-ops (Team:ResponseOps) |
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.
Looks straightforward 😅
// In the scenario the task is unused / deprecated and Kibana needs to manipulate the task, | ||
// we'll do a pass-through for those | ||
if (!this.definitions.has(task.taskType)) { |
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.
surprised there's no unit test for validating an unknown task type to update. Maybe we should add one?
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.
Do these not cover that?
kibana/x-pack/plugins/task_manager/server/task_validator.test.ts
Lines 29 to 40 in e49e89e
describe('getValidatedTaskInstanceFromReading()', () => { | |
it(`should return the task as-is whenever the task definition isn't defined`, () => { | |
const definitions = new TaskTypeDictionary(mockLogger()); | |
const taskValidator = new TaskValidator({ | |
logger: mockLogger(), | |
definitions, | |
allowReadingInvalidState: false, | |
}); | |
const task = taskManagerMock.createTask(); | |
const result = taskValidator.getValidatedTaskInstanceFromReading(task); | |
expect(result).toEqual(task); | |
}); |
kibana/x-pack/plugins/task_manager/server/task_validator.test.ts
Lines 287 to 298 in e49e89e
describe('getValidatedTaskInstanceForUpdating()', () => { | |
it(`should return the task as-is whenever the task definition isn't defined`, () => { | |
const definitions = new TaskTypeDictionary(mockLogger()); | |
const taskValidator = new TaskValidator({ | |
logger: mockLogger(), | |
definitions, | |
allowReadingInvalidState: false, | |
}); | |
const task = taskManagerMock.createTask(); | |
const result = taskValidator.getValidatedTaskInstanceForUpdating(task); | |
expect(result).toEqual(task); | |
}); |
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.
Oh I see. I didn't realize it was using definitions.has
which doesn't throw anyway not definitions.get
.
/ci |
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!
Can't tell what's going on with the build errors - looks systematic, maybe ES didn't come up correctly? Going to start merge main to run again, and poke around those tests a bit ... |
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]
History
To update your PR or re-run it, just comment with: |
resolves #189560
Summary
Removes the
throw
fromTaskTypeDictionary::get()
and have it returnTaskDefinition | undefined
instead.Then fix all the places that breaks. This could probably never happen when a task is running - the definition would be needed for a bunch of things prior to this (like polling for the task). However, it has occurred during metric generation, and likely more code in the future will use this, so the new API has less of a "suprise! Error!" aspect to it, for such an innocent looking method name.
Several places in the existing code did not have to be changed as they already guards for checking null-ish returns from the method :-)