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.

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

Moves Task manager's interval under a generic schedule field #52727

Merged
merged 61 commits into from
Dec 17, 2019

Conversation

gmmorris
Copy link
Contributor

@gmmorris gmmorris commented Dec 11, 2019

Summary

This moves the interval field under a generic schedule object field in preparation for the introduction of richer scheduling options (such as cron).

It includes a migration for existing tasks, and we've ensured no existing Task Type Definitions exist in Kibana that rely on Interval.

This includes support for the deprecated interval field (which gets mapped to schedule) but that support will be removed in 8.0.0, as it's a breaking change.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

gmmorris and others added 30 commits November 25, 2019 13:38
@gmmorris
Copy link
Contributor Author

@elasticmachine merge upstream

@gmmorris gmmorris added the Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) label Dec 13, 2019
Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@mikecote mikecote left a comment

Choose a reason for hiding this comment

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

Great work 👍 LGTM, just a few nits.

@@ -14,7 +14,9 @@ import {
RangeBoolClause,
} from './query_clauses';

export const RecuringTaskWithInterval: ExistsBoolClause = { exists: { field: 'task.interval' } };
export const TaskWithRecurringSchedule: ExistsBoolClause = {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: TaskWithSchedule

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No a nit at all, we changed that name and I missed it :)
Thanks!

@@ -91,7 +94,7 @@ export function initRoutes(server, taskTestingEvents) {
payload: Joi.object({
task: Joi.object({
taskType: Joi.string().required(),
interval: Joi.string().optional(),
schedule: Joi.string().optional(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
schedule: Joi.string().optional(),
schedule: Joi.object({
interval: Joi.string(),
}).optional(),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

eek why wasn't that caught by functional test?? Investigating

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahhh because we don't send a new interval on the ensure api call.... that's why.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need that in that API call (our type ensures this is in the actual TM) in our FTs so this'll be removed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

History

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

@gmmorris gmmorris merged commit 2b6ef5c into elastic:master Dec 17, 2019
gmmorris added a commit to gmmorris/kibana that referenced this pull request Dec 17, 2019
…#52727)

This moves the interval field under a generic schedule object field in preparation for the introduction of richer scheduling options (such as cron).

It includes a migration for existing tasks, and we've ensured no existing Task Type Definitions exist in Kibana that rely on Interval.

This includes support for the deprecated interval field (which gets mapped to schedule) but that support will be removed in 8.0.0, as it's a breaking change.
gmmorris added a commit that referenced this pull request Dec 17, 2019
…#53295)

This moves the interval field under a generic schedule object field in preparation for the introduction of richer scheduling options (such as cron).

It includes a migration for existing tasks, and we've ensured no existing Task Type Definitions exist in Kibana that rely on Interval.

This includes support for the deprecated interval field (which gets mapped to schedule) but that support will be removed in 8.0.0, as it's a breaking change.
gmmorris added a commit that referenced this pull request Dec 18, 2019
…52873)

Follow up from the #52727 in Task Manager, we want Alerting and Task Manager to align on their schedule api (in the near future, Alerting will actually use Task manager's schedule system to remove this duplication).
gmmorris added a commit to gmmorris/kibana that referenced this pull request Dec 18, 2019
…lastic#52873)

Follow up from the elastic#52727 in Task Manager, we want Alerting and Task Manager to align on their schedule api (in the near future, Alerting will actually use Task manager's schedule system to remove this duplication).
mikecote pushed a commit that referenced this pull request Dec 18, 2019
…52873) (#53515)

Follow up from the #52727 in Task Manager, we want Alerting and Task Manager to align on their schedule api (in the near future, Alerting will actually use Task manager's schedule system to remove this duplication).
@mikecote
Copy link
Contributor

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported Feature:Task Manager release_note:enhancement Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.6.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants