-
Notifications
You must be signed in to change notification settings - Fork 63
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
support task-schema refactor #217
Conversation
535b640
to
bcf2c7b
Compare
Hi @yyscamper. I'm curious where this PR stands at the moment. Do you intend to make any additional changes such as those we discussed during your visit? |
@brianparry No additional change, I was trying to resolve the conflict and rebase my code to make sure the dependency Jenkins passes, then I will base on this code to do stress testing. This is the reason why you see the Jenkins becomes active again. |
Looks good to me. 👍 |
BUILD on-core #964 : FAILURE
|
test this please |
BUILD on-core #966 : FAILURE
|
test this please |
1 similar comment
test this please |
@yyscamper The change looks good, so 👍 , but I think we should consider adding parameter constraints inline with task definition properties rather than defining a seperate schema property. One way to think about this is that the task definitions themselves would be a kind of schema. I think this would make writing task definitions easier for users. |
@brianparry : Thanks for your review. But I don't quite understand your comment, whether you mean the schema should go along with each option rather than put together in a separate property? Something like below? {
injectableName: 'Task.InstallCentOS',
friendlyName: 'CentOS install',
options: {
version: {
type: 'string',
required: true
},
repo: {
type: 'string',
required: true,
default: '{{api.server}}/centos/{{options.version}}'
}
}
} I did think about this, the challenge is it cannot or hard meet some complex case, for example, for LinuxCommand task, the options: {
command: {
oneOf:[
{
type: 'string'
},
{
type: 'object',
properties: {
...
}
}
]
}
} It just like one task definition contains multiple sub json schema. For my case, all sub json schemas are contained in the seperated key Another reason is for some complex oneOf: [
{ 'required': ['controllerId', 'createDefault'] }
{ 'required': ['controllerId', 'raidConfig'] }
] This kind of constraint applies for the whole of options rather than a single option. Put all constraints together gives us the flexibility to use all JSON schema capability. Split the whole schema into multiple sub schemas does have some constraints to handle some complex case. If you have better solution to resolve above complex case that will be very welcome. |
…a for JSON validator
bcf2c7b
to
222cf39
Compare
+1 |
Change including:
(1) Task-Definition: replace schemaRef with optionsSchema, optionsSchema can be either string or object, so set its type to
json
.(2) add removeSchema API for JSON schema validator;
@RackHD/corecommitters @iceiilin @pengz1 @cgx027 @lanchongyizu @amymullins