-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
fix(task): support tasks without commands #27191
Conversation
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.
Marking as "Request changes" so we don't land it by accident before v2.2 merge window.
@@ -446,7 +446,6 @@ | |||
}, | |||
"command": { | |||
"type": "string", | |||
"required": true, | |||
"description": "The task to execute" |
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.
Can you update the description to mention that this field is required unless there's "dependencies"
field specified?
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.
Is it required though? Having a task like this is valid in the string form:
{
"tasks": {
"foo": ""
}
}
And I guess the same is also fine for the object notation:
{
"tasks": {
"foo": {}
}
}
That's just a task that does nothing.
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. Kind of feels like we could just merge this as a fix.
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'm fine merging it as a fix. Please update docs before merging it though
Support running tasks that have no command and only dependencies. This is useful for when you want to group tasks only.
Fixes #27165