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

Support Dot and Backet Notation in parameters #345

Open
Tracked by #297
tlawrie opened this issue May 9, 2022 · 7 comments
Open
Tracked by #297

Support Dot and Backet Notation in parameters #345

tlawrie opened this issue May 9, 2022 · 7 comments
Assignees

Comments

@tlawrie
Copy link
Member

tlawrie commented May 9, 2022

In Tekton 0.28.x and 0.29.y support was added for Parameters with . notation. tektoncd/pipeline#4215

As well as using [] with both single and double quotes tektoncd/pipeline#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.

@tlawrie tlawrie moved this to To Do in Boomerang Flow May 9, 2022
@tlawrie tlawrie changed the title Support Dot Notation in parameters Support Dot and Backet Notation in parameters May 9, 2022
@timrbula
Copy link
Member

timrbula commented Jun 7, 2022

@tlawrie To find the associated TEP for Tekton

@timrbula
Copy link
Member

timrbula commented Jun 7, 2022

@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:

  • dot notation
  • bracket notation w/ multiple values and "" and '' value quoting

@tlawrie
Copy link
Member Author

tlawrie commented Jun 21, 2022

@tlawrie to look at PR from @amhudson

@tlawrie
Copy link
Member Author

tlawrie commented Jun 28, 2022

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

  • Dot Notation = $(params.myparam)
  • Bracket Notation = $(params['myparam']) and $(params["myparam"])

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

  • I think we would have to make sure that at every point in the database it's stored with the KeyValuePair
  • I see there is a check on . in the PR.

Based on Specifying Parameters Parameter names:

  • Must only contain alphanumeric characters, hyphens (-), underscores (_), and dots (.).
  • Must begin with a letter or an underscore (_).

If a parameter name contains dots (.), it must be referenced by using the bracket notation with either single or double quotes i.e. $(params['foo.bar']), $(params["foo.bar"]). See the following example for more information.

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

  • dot
  • bracketSingleQuote
  • bracketDoubleQuote
    My concern is how that works with passing it around the system and when we convert back and forth into a Map

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
Copy link
Member Author

tlawrie commented Jul 6, 2022

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

@amhudson
Copy link
Member

amhudson commented Jul 6, 2022

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

@amhudson
Copy link
Member

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

No branches or pull requests

6 participants