-
Notifications
You must be signed in to change notification settings - Fork 266
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
Add task.json schema file #146
Conversation
Eric and Yash, can you review the schema file? |
"Node": { | ||
"$ref": "#/definitions/executionObject" | ||
}, | ||
"Bash": { |
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.
Bash isn't an execution handler.
Not required in this merge, but I'm inclined to remove the ones that we really don't advertise for custom tasks (i.e. leave Node and PowerShell3 only). I might delete the other execution handlers from the schema after the merge and add them back as needed (hopefully not needed).
Also not all of the execution objects properties are the same for all handlers. I can track down those details after the merge.
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.
Thanks, @ericsciple. I developed this JSON schema to validate all the tasks in the vsts-tasks repo. So if you see values that aren't supposed to be there, there is probably some task in that repo that uses that value, just FYI.
@bryanmacfarlane do you have any thoughts where the file should go? I'm guessing once we choose a location, we need to leave it there. This is nice. |
@AArnott fyi - i haven't forgotten about this one... lot going on right now, shutting down for RTM and normal 3wk sprints going on in parallel. |
@bryanmacfarlane are you fine with the location of this file (root of the repo)? i'm guessing once we choose a location it needs to stay there (since folks will be hardcoding a dependency to that specific url) |
tasks.schema.json
Outdated
"pickList", | ||
"radio", | ||
"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.
Because you can add new "connectedService:" within an extension, I think this enum needs to be:
"anyOf": [
{
"enum": [
"boolean",
"filePath",
"multiLine",
"pickList",
"radio",
"string"
]
},
{
"pattern": "^connectedService\\:.*$"
}
]
This still gives the intellisense for the standard inputs, but also avoids validation errors for new connectedServices. Adding known connectedServices to the enum would also be reasonable.
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.
Good idea. I'd fix this, but would like to know if this PR isn't just dead first. So far, it seems to be that no one with commit privileges is acting on 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.
Which is silly, considering the official Microsoft documentation still references your schema.
@AArnott The main concern I have is, once it is merged, then is the location sealed? Or would an fwlink work in the |
I suspect json language services can handle HTTP redirects, but I haven't tried it. FWIW it looks like folks are already burning in a specific commit's version of this schema, so by completing the PR and updating the docs we at least get them to 'tip' of whatever you have. But assuming fwlink's work (or perhaps since this is customer visible an aka.ms link would be better) are you ok with merging this then? |
OK, I just tested it. VS Code (for one) supports redirects in their JSON schema URLs. I set up https://aka.ms/vsts-tasks.schema.json and made you, @ericsciple, co-owner of it so you can update it once this PR completes. |
OK my last concern is how do folks know they should use the aka link, and not whatever URL they happen to be browsing when they come across the schema document. It looks like json schema supports an "id" property that can be used to identify the URL of the schema document itself, e.g. {
"id": "https://aka.ms/vsts-tasks.schema.json" So that addresses my last concern. |
@bryanmacfarlane any outstanding concerns? I can add the ID at the top after merged, or whatever. |
Ping. I'm seeing random folks on github referencing this .json file from my fork. The sooner this PR gets completed (or guidance for placing it elsewhere is given), the sooner folks can reference the official location and it can be properly maintained. |
Is this the PR you were referring to, @bryanmacfarlane? |
"type": "string", | ||
"description": "The target file to be executed. You can use variables here in brackets e.g. $(currentDirectory)\filename.ps1" | ||
}, | ||
"argumentFormat": { |
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.
@bryanmacfarlane Is there a description for argumentFormat? I've searched but have not found what argument format is suppose to do. For most examples is just an empty string. Fox example: Can I pass arguments here?
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 is from legacy handlers and it should not be used.
The id https://aka.ms/vsts-tasks.schema.json currently redirects to the wrong place. It points at https://github.com/Microsoft/vsts-task-lib/blob/master/tasks.schema.json but needs to point at https://raw.githubusercontent.com/Microsoft/vsts-task-lib/master/tasks.schema.json |
thanks, will fix |
Fixes microsoft/azure-pipelines-tasks#1854