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

Support Dot and Backet Notation in parameters #345

Open tlawrie opened 2 years ago

tlawrie commented 2 years ago

In Tekton 0.28.x and 0.29.y support was added for Parameters with . notation. https://github.com/tektoncd/pipeline/pull/4215

As well as using [] with both single and double quotes https://github.com/tektoncd/pipeline/pull/4268

For example, the following are equivalent: $(param.myparam), $(param['myparam']), and $(param["myparam"]). Bracket notation has the additional benefit of allowing users to work with parameter names containing conflicting characters like "." (e.g. $(param['my.param']) or $(param["my.param"]).

As part of this, we need to investigate whether we can support parameters with . notation. This would also then be good to have scopes back in our parameters (as well with any solutions extended Flow).

We could then have System, Global, Team, etc prefixed / scoped parameters.

What changes would be needed in the service (and storage in the DB as .'s do mess with mongo if not handled correctly) and also in the front end to allow entry of a dot.

timrbula commented 2 years ago

@tlawrie To find the associated TEP for Tekton

timrbula commented 2 years ago

@BenjaminRuby Can you provide some context on this from the platform's perspective? Tyson mentioned that supporting multiselect was a requested feature.

@amhudson Can you assess the technical feasibility of supporting both for accessing parameters:

tlawrie commented 2 years ago

@tlawrie to look at PR from @amhudson

tlawrie commented 2 years ago

Hi @amhudson looking at the PR, I just want to make sure we have covered all the cases.

References When I refer to the notations in the following, it means

Current State Right now, the UI only deals with the parameters in standard dot notation.

At the moment, the only time that new parameters could enter the system is in the YAML import of a task definition, or in the Results from a task when it outputs an array type (which we dont yet support either). although it would allow us to add support for multi-select in a Workflows Parameters.

Character Support

Based on Specifying Parameters Parameter names:

However, that doesn't guarantee people didn't use bracket notation on a standard parameter name

Entry Points Entry points for referencing these new parameter formats. How do we decide what format came in and therefore what format to show the user / let the user use in reference?

What would happen if someone creates a task with parameter format reference of $(params["myparam"]), we store it in the DB, and then translate that back to $(params.myparam)?

Do we have to store a new element on a ParamKeyValuePair object that stores the notation? not sure how else to know of which we have an enum, based on the terminology of

Parameters can be of the type String or Array We never added support for arrays, if we create a special class for Parameters of ParameterKeyValuePair (which may extend KeyValuePair), then we could also add a type of string or array.

This would allow support for multi-select creation in the UI too. @timrbula @BenjaminRuby (had a separate PR that's been on hold for this).

Array support would mean we also have to handle * in $(params.flags[*])

Recommend: we cater for it by do in a separate issue as it has UI impact

Results I believe the same notation support has to be done for Task / Workflow Results -> https://tekton.dev/docs/pipelines/variables/

Pull Request If I check on what uses KeyValuePair, a few additional areas of the app come up. There seems to be a few places that we map from KeyValuePair into a Map<String, String>, which will that handle . and arrays etc?

We also have a PropertyManagerImpl which might be good to put all of this into? Not sure if thats internal vs external consumption

I think we just have to make sure, that if we update the model to deal with all this, then we have to figure out a. how it impacts anytime we convert into a Map or vice-versa. b. how we pass it arround as a Map which would mean we lose the notation... if we do that

tlawrie commented 2 years ago

Hey @amhudson have you been able to take a look at the above?

amhudson commented 2 years ago

yea, but I haven't been able to circle back to it.

amhudson commented 1 year ago

@tlawrie I have support for the following:

workflow.params['my.param'] params['my.param'] workflow.params[\"my.param\"] params[\"my.param\"]

Which notation(s) am I missing and can you provide me an example of how it should appear?