Task schema refactor #360
Task schema refactor #360
Conversation
|
The travisci and jenkins failure is expected, since it depends on the change of on-core |
|
test this please |
|
This is still a -1 for me. The complexity of the design/redesign continues to carry a very large development/support, usability, and long term maintenance cost that is highly disproportionate to its business value for API Input Validation. Several simplification suggestions were provided in previous Core Committer meetings and email threads. I request we revisit the topic. |
yyscamper
force-pushed
the
task-schema-refactor
branch
from
e750fcb to
26ff50c
Compare
|
BUILD on-tasks #1774 : UNSTABLE
BUILD unit-tests #10280 Error Logs ▼Test Name: reset-spec.js task-data expected properties should have valid default option values Error Details: validator.removeSchema is not a function Stack Trace: TypeError: validator.removeSchema is not a function at Context.<anonymous> (spec/lib/task-data/tasks/base-tasks-spec.js:115:27)Test Name: clear-sel-spec.js task-data expected properties should have valid default option values Test Name: catalog-lsall-spec.js task-data expected properties should have valid default option values Test Name: shell-commands-spec.js task-data expected properties should have valid default option values Test Name: flash-ami-spec.js task-data expected properties should have valid default option values Test Name: catalog-perccli-spec.js task-data expected properties should have valid default option values Test Name: download-files-spec.js task-data expected properties should have valid default option values Test Name: catalog-local-lldp-spec.js task-data expected properties should have valid default option values Test Name: provide-quanta-bios-version-spec.js task-data expected properties should have valid default option values Test Name: noop-task-spec.js task-data expected properties should have valid default option values Test Name: catalog-mgmt-bmc-spec.js task-data expected properties should have valid default option values Test Name: catalog-lldp-spec.js task-data expected properties should have valid default option values Test Name: reboot-spec.js task-data expected properties should have valid default option values Test Name: delete-megaraid-spec.js task-data expected properties should have valid default option values Test Name: rm-bmc-credentials-spec.js task-data expected properties should have valid default option values Test Name: catalog-ami-spec.js task-data expected properties should have valid default option values Test Name: catalog-local-dmi-spec.js task-data expected properties should have valid default option values Test Name: catalog-megaraid-spec.js task-data expected properties should have valid default option values Test Name: catalog-driveid-spec.js task-data expected properties should have valid default option values Test Name: catalog-bmc-spec.js task-data expected properties should have valid default option values Test Name: soft-reset-spec.js task-data expected properties should have valid default option values Test Name: set-bmc-credentials-spec.js task-data expected properties should have valid default option values Test Name: provide-catalog-ami-bios-version-spec.js task-data expected properties should have valid default option values Test Name: identify-off-spec.js task-data expected properties should have valid default option values Test Name: power-on-spec.js task-data expected properties should have valid default option values Test Name: catalog-dmi-spec.js task-data expected properties should have valid default option values Test Name: boot-profile-spec.js task-data expected properties should have valid default option values Test Name: catalog-ohai-spec.js task-data expected properties should have valid default option values Test Name: catalog-flashupdt-spec.js task-data expected properties should have valid default option values Test Name: identify-on-spec.js task-data expected properties should have valid default option values Test Name: flash-megaraid-spec.js task-data expected properties should have valid default option values Test Name: set-ami-nvram-spec.js task-data expected properties should have valid default option values Test Name: analyze-esx-repo-spec.js task-data expected properties should have valid default option values Test Name: catalog-smart-spec.js task-data expected properties should have valid default option values Test Name: set-boot-pxe-spec.js task-data expected properties should have valid default option values Test Name: create-megaraid-spec.js task-data expected properties should have valid default option values Test Name: flash-quanta-bmc-spec.js task-data expected properties should have valid default option values Test Name: condition-spec.js task-data expected properties should have valid default option values Test Name: MC Reset Cold task-data expected properties should have valid default option values Test Name: power-off-spec.js task-data expected properties should have valid default option values BUILD unit-tests #10281 Error Logs ▼Test Name: Graph Library should validate all existing graphs not requiring user input for null values Error Details: Task.Inline.Catalog.Switch.Arista: JSON schema validation failed - data.commands[0] should be string, data.commands[0].downloadUrl should match pattern "^/[a-zA-Z0-9+-.#?=\/]+", data.commands[0] should match exactly one schema in oneOf, data.commands should be string, data.commands should be object, data.commands should match exactly one schema in oneOf, data.commands should match exactly one schema in oneOf Stack Trace: Error: Task.Inline.Catalog.Switch.Arista: JSON schema validation failed - data.commands[0] should be string, data.commands[0].downloadUrl should match pattern "^/[a-zA-Z0-9+-.#?=\/]+", data.commands[0] should match exactly one schema in oneOf, data.commands should be string, data.commands should be object, data.commands should match exactly one schema in oneOf, data.commands should match exactly one schema in oneOf at TaskOptionValidator.JsonSchemaValidator.validatePatternsSkipped (node_modules/on-core/lib/common/json-schema-validator.js:94:23) at TaskOptionValidator.validateContextSkipped (node_modules/on-tasks/lib/utils/task-option-validator.js:61:21) at node_modules/on-tasks/lib/task.js:619:31 at arrayEach (node_modules/lodash/index.js:1289:13) at Function.<anonymous> (node_modules/lodash/index.js:3345:13) at Function.Task.validateOptions (node_modules/on-tasks/lib/task.js:616:11) at node_modules/on-tasks/lib/task.js:437:18 at tryCatcher (node_modules/on-core/node_modules/bluebird/js/main/util.js:26:23) at Promise._settlePromiseFromHandler (node_modules/on-core/node_modules/bluebird/js/main/promise.js:510:31) at Promise._settlePromiseAt (node_modules/on-core/node_modules/bluebird/js/main/promise.js:584:18) at Promise._settlePromises (node_modules/on-core/node_modules/bluebird/js/main/promise.js:700:14) at Async._drainQueue (node_modules/on-core/node_modules/bluebird/js/main/async.js:123:16) at Async._drainQueues (node_modules/on-core/node_modules/bluebird/js/main/async.js:133:10) at Immediate.Async.drainQueues [as _onImmediate] (node_modules/on-core/node_modules/bluebird/js/main/async.js:15:14)BUILD on-taskgraph #1763 Error Logs ▼Test Name: Graph Library should validate all existing graphs not requiring user input for null values Error Details: Task.Inline.Catalog.Switch.Arista: JSON schema validation failed - data.commands[0] should be string, data.commands[0].downloadUrl should match pattern "^/[a-zA-Z0-9+-.#?=\/]+", data.commands[0] should match exactly one schema in oneOf, data.commands should be string, data.commands should be object, data.commands should match exactly one schema in oneOf, data.commands should match exactly one schema in oneOf Stack Trace: Error: Task.Inline.Catalog.Switch.Arista: JSON schema validation failed - data.commands[0] should be string, data.commands[0].downloadUrl should match pattern "^/[a-zA-Z0-9+-.#?=\/]+", data.commands[0] should match exactly one schema in oneOf, data.commands should be string, data.commands should be object, data.commands should match exactly one schema in oneOf, data.commands should match exactly one schema in oneOf at TaskOptionValidator.JsonSchemaValidator.validatePatternsSkipped (node_modules/on-core/lib/common/json-schema-validator.js:94:23) at TaskOptionValidator.validateContextSkipped (node_modules/on-tasks/lib/utils/task-option-validator.js:61:21) at node_modules/on-tasks/lib/task.js:619:31 at arrayEach (node_modules/lodash/index.js:1289:13) at Function.<anonymous> (node_modules/lodash/index.js:3345:13) at Function.Task.validateOptions (node_modules/on-tasks/lib/task.js:616:11) at node_modules/on-tasks/lib/task.js:437:18 at tryCatcher (node_modules/on-core/node_modules/bluebird/js/main/util.js:26:23) at Promise._settlePromiseFromHandler (node_modules/on-core/node_modules/bluebird/js/main/promise.js:510:31) at Promise._settlePromiseAt (node_modules/on-core/node_modules/bluebird/js/main/promise.js:584:18) at Promise._settlePromises (node_modules/on-core/node_modules/bluebird/js/main/promise.js:700:14) at Async._drainQueue (node_modules/on-core/node_modules/bluebird/js/main/async.js:123:16) at Async._drainQueues (node_modules/on-core/node_modules/bluebird/js/main/async.js:133:10) at Immediate.Async.drainQueues [as _onImmediate] (node_modules/on-core/node_modules/bluebird/js/main/async.js:15:14)BUILD unit-tests #10282 Error Logs ▼Test Name: Http.Services.Api.Workflows "before each" hook for "should create and run a graph not against a node" Error Details: Cannot stub non-existent own property updateGraphProgress Stack Trace: TypeError: Cannot stub non-existent own property updateGraphProgress at Object.stub (node_modules/sinon/lib/sinon/collection.js:93:35) at Context.<anonymous> (spec/lib/services/workflow-api-service-spec.js:79:22)Test Name: Http.Api.Notification POST /notification should update graph progress Test Name: Http.Api.Notification POST /notification should not update graph progress BUILD on-http #2903 Error Logs ▼Test Name: Http.Services.Api.Workflows "before each" hook for "should create and run a graph not against a node" Error Details: Cannot stub non-existent own property updateGraphProgress Stack Trace: TypeError: Cannot stub non-existent own property updateGraphProgress at Object.stub (node_modules/sinon/lib/sinon/collection.js:93:35) at Context.<anonymous> (spec/lib/services/workflow-api-service-spec.js:79:22)Test Name: Http.Api.Notification POST /notification should update graph progress Test Name: Http.Api.Notification POST /notification should not update graph progress |
brianparry
left a comment
brianparry
left a comment
There was a problem hiding this comment.
Hi @yyscamper. I think this patch is definite improvement over what we have today and should address most of the issues. I would prefer to have the constraints on each option defined inline which would essentially make the definition a schema in itself, but I think this approach is acceptable. My biggest concern is that base jobs are still included in the design. Essentially they have just become a tool to relate schemas to jobs. Since job schema is tightly coupled to job code, I don't think base-jobs are well suited for this. I think the base-job abstraction is no longer necessary with this design and will end up making things more confusing for users trying to write custom tasks.
| throw new Error( (title ? (title + ': ') : '') + 'schema must be either string or object'); | ||
| } | ||
|
|
||
| Task.getBaseTask = function(query) { |
There was a problem hiding this comment.
@yyscamper As we discussed during your visit, I think we should be getting rid of base tasks altogether. Is this part of the long term plan for this design?
|
@brianparry : For why I don't choose the inline schema for each option, I have replied you in the on-core PR: RackHD/on-core#217 As for why I still keep the base-task, I explained using the MVC. The baseTask is designed as the "viewer" layer of job, user don't need to read the job code but can still know how to use the job. Though now the baseTask only contains the job schema, but it can be easily to extend to put some annotation here in future, for example: Of course, all above bases on my thought on future baseTask, and I agree the baseTask is not the only way for above future features ( if we agree it is a valid feature). At present, the biggest problem to keep baseTask is it is easy to diverge because the job code and job schema are put in different source folder, developer may forget to change both. In a whole, I am not against to remove the baseTask, If we all agree it's not necessary to keep baseTask, I can put it into my plan to remove it. Actually, to remove the baseTask (and moving job schema to job code itself) will introduce a lot change including:
So my suggestion is even we decide to remove baseTask, let's keep it in another PR. |
|
test this please |
|
BUILD on-multi #15 : FAILURE
|
|
test this please |
|
BUILD on-multi #16 : FAILURE
|
|
@yyscamper Specifying tooling requirements is an interesting use of base jobs that I hadn't considered. Although, don't you still have the problem of the job code and job specification living in two different places where one could be changed without the other? In any case, I'm okay with discussing that separately and making that another PR, @amymullins What are your thoughts here? Any objection to deferring the removal of base jobs? |
|
@brianparry and @yyscamper I have no objection to the deferral. Removal of the base jobs was part of the original discussions/requirements, so we probably won't want to defer for too long. |
brianparry
left a comment
brianparry
left a comment
There was a problem hiding this comment.
👍 but please make sure to run regression tests against this branch before merging.
1. split into common/job/task-specific schemas 2. add a key 'optionsSchema' in both task and baseTask definition to store schema. 3. job/task-specific schema is optional 4. support built-in schema and reference external schema file. 5. refactor install-os schemas design. 6. add schema for Windows server installation. 7. change unit-test accordingly. 8. change task-annotation accordingly.
yyscamper
force-pushed
the
task-schema-refactor
branch
from
26ff50c to
9e92da4
Compare
|
+1 as long as all tests run smoothly |
8 participants
NOTE
This is a pretty big PR, sorry to bring some trouble for reviewer. I intentionally create this big PR to combine all changes about the task-schema refactor design because I don't want to bring confusion for user & developer if both old schema and new schema coexist.
Actually the core code is in my POC (see #354), you can first get the general structure design about new task-schema from POC. The most additional work of this PR comparing with the POC is just to port all existing schema to new one, so you will see most code change is similar.
Change List
Following is the change list for this PR:
1. split into common/job/task-specific schemas
2. add a key 'optionsSchema' in both task and baseTask definition to store schema.
3. job/task-specific schema is optional
4. support built-in schema and reference external schema file.
5. refactor install-os schemas design.
6. add schema for Windows server installation.
7. change unit-test accordingly.
8. change task-annotation accordingly.
@RackHD/corecommitters @iceiilin @pengz1 @cgx027 @lanchongyizu @amymullins
@tannoa2 : I also added the schema for windows installation, please help review it, the file is: https://github.com/yyscamper/on-tasks/blob/task-schema-refactor/lib/task-data/schemas/install-windows.json
jenkins: depends on RackHD/on-core#217 RackHD/on-taskgraph#176