boomerang-io / community

The Boomerang community, roadmap, planning, and architecture repository. The central place for information on joining, contributing, and governance.
https://useboomerang.io
Apache License 2.0
11 stars 0 forks source link

Workflow + Engine - Fix up the use of config vs params #437

Open tlawrie opened 1 month ago

tlawrie commented 1 month ago

Two remaining concerns with the current params and config implementation

Potential Solutions Store config in the annotations, or merge with an extended params that can then be easily whittled down for the tekton params. If merging, then config.type could become element as its the UI element to be rendered.

Potential Mapping of types Config Type Param Type Value Would it work?
text string "text" yes
textArea string "this is a\nmultiline\nstring\nad\nasd\nad" yes
password string "" yes
number string "2" yes
boolean string "true" yes
email string "test@test.com" yes
url string "https://test.com" yes
select array ["--set", "arg1=foo", "--randomflag", "--someotherflag"] partially. Config has 'options' which are label: value pairs. rather than just a list which is the Tekton param. Potentially we make this richer and then cut down when converting to Tekton otherwise a cync would have to be maintained
- object - The Tekton object type is very different to what we have implemented which is a Java object. Note: This has been tested via the API for creating a workflow and setting JSON as the value to an object type. Uncertain how to render in the UI, what element would it map to

References

tlawrie commented 1 month ago

We alos have to update the various implementations of this conversion.

Its done at canvas and tekton conversion time in multiple different ways.

tlawrie commented 1 month ago

Once this change is made, will need to re-validate the secure params as well.

tlawrie commented 1 month ago

@timrbula if you could review the above and have a think about the two types that only partially match. As well as whether we should do a merged object or not.

Option for a Merged Schema

"params": [
    {
        "name" : "secret",
        "type" : "string",
        "description" : "",
        "default" : "",
        "config": {
              "key" : "secret", //this could be removed in favour of param.name
              "description" : "", //this could be removed in favour param.description
              "label" : "My Secret Password",
              "type" : "password", //this **could** become config.element
              "required" : false,
              "placeholder" : "",
              "helperText" : "",
              "minValueLength" : "",
              "maxValueLength" : "",
              "language" : "",
              "disabled" : "",
              "defaultValue" : "", ///this could be removed in favor of param.default
              "value" : "",
              "values" : "", //not sure what this is used for
              "readOnly" : false,
              "hiddenValue" : false,
              "options" : [
                  {
                      "key" : "asdads",
                      "value" : "asdads"
                  },
              ]
          }
    },
   ...
]

Benefits

  1. The API will only expect the require param.name, param.type
  2. The tekton convertor will drop the param.config object
  3. The canvas convertor will be able to create a new flattened object or just send this one

Drawbacks