Skip to content
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

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

Open
tlawrie opened this issue Aug 7, 2024 · 3 comments
Open

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

tlawrie opened this issue Aug 7, 2024 · 3 comments
Assignees
Labels
bug Something isn't working

Comments

@tlawrie
Copy link
Member

tlawrie commented Aug 7, 2024

Two remaining concerns with the current params and config implementation

  • We make an assumption that config is source of truth on the canvas endpoint, and params is the source of truth on Workflows endpoints
  • Param type does not equal end user type (i.e. params can only be string, array, object).

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 "[email protected]" 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 tlawrie converted this from a draft issue Aug 7, 2024
@tlawrie tlawrie added the bug Something isn't working label Aug 7, 2024
@tlawrie
Copy link
Member Author

tlawrie commented Aug 8, 2024

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

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

@tlawrie
Copy link
Member Author

tlawrie commented Aug 8, 2024

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

@tlawrie
Copy link
Member Author

tlawrie commented Aug 8, 2024

@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

  • Potential impact to the UI and all the stored Workflows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Ready
Development

No branches or pull requests

2 participants