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

Enum Parameter Type #2610

Closed
andrewballantyne opened this issue May 12, 2020 · 12 comments
Closed

Enum Parameter Type #2610

andrewballantyne opened this issue May 12, 2020 · 12 comments
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.

Comments

@andrewballantyne
Copy link

Some of the template sections are not feature-request oriented, so I'll just give a trimmed down version. Today this does not exist as far as I can tell from the docs. There is a suggestion in #1393 (comment) but it's asking for a lot of other flexibility as well. Which might be a larger and slower to get ask.

Expected Behavior

params:
  # Existing today
  - name: cat-name
    description: Your cat name
    type: string
  - name: nicknames
    description: Nicknames for your cat
    type: array

  # New
  - name: breed
    description: The breed of your cat
    type: enum
    default: not-sure
    values:
      - persian
      - siamese
      - himalayan
      - not-sure

I believe this format works for Pipeline params, Task params, and maybe even worth going to TriggerTemplates on the Trigger package if viable.

Additional Info

Expectation would be for the logic to match 1:1 each values option. Not sure if it needs to be only strings but since we don't really have non-string types today not sure there is a concern here. Either way, I think it can just be a 1:1 equality check against the list of items and if the types grow, it should "just strong equality match". So if support of Booleans or Numbers come down the line, it won't be a problem.

@bobcatfish
Copy link
Collaborator

@skaegi probably has some thoughts here, he's been working on adding something like a "schema" to params to allow for many different types beyond just the array and string we currently have

@vdemeester
Copy link
Member

/kind feature

@tekton-robot tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label May 13, 2020
@bobcatfish
Copy link
Collaborator

I'll add this to the agenda of our API working group on Monday to get some more eyes on it and see where the schema work (#1393) is at.

@skaegi
Copy link
Contributor

skaegi commented May 13, 2020

For validation of params it's a bit of a balancing act as we want to retain backwards-compatibility at the same time as improving validation. For the validation in particular I'm hesitant to re-invent syntax here so what I'm thinking of borrows a lot from json schema. (See https://tools.ietf.org/html/draft-handrews-json-schema-validation-02#section-6.1.2)

For enum I'd suggest something like...

params:
  # New
  - name: breed
    description: The breed of your cat
    type: string
    default: not-sure
    schema:
      enum:
        - persian
        - siamese
        - himalayan
        - not-sure

(e.g. leave all schema aspects in a schema property)
or

params:
  # New
  - name: breed
    description: The breed of your cat
    type: string
    default: not-sure
    enum:
      - persian
      - siamese
      - himalayan
      - not-sure

(e.g. add peer schema aspects directly to the param [might be hard to support long term])

@skaegi
Copy link
Contributor

skaegi commented May 13, 2020

@andrewballantyne there is still a ton of work to actually support multiple types as per #1393 however we could still make progress on the validation aspect independently. e.g. schema could be implemented right now I believe.

@andrewballantyne
Copy link
Author

params:
  # New
  - name: breed
    description: The breed of your cat
    type: string
    default: not-sure
    schema:
      enum:
        - persian
        - siamese
        - himalayan
        - not-sure

This is definitely more open-ended and can evolve in the future to support anything. I think this is probably the better of the two solutions (and for the support of #1393 and it's goals).

params:
  # New
  - name: breed
    description: The breed of your cat
    type: string
    default: not-sure
    enum:
      - persian
      - siamese
      - himalayan
      - not-sure

This definitely aligns more with what I was thinking, but I wonder if it's a little less long-lasting.

@andrewballantyne there is still a ton of work to actually support multiple types as per #1393 however we could still make progress on the validation aspect independently. e.g. schema could be implemented right now I believe.

This feature request was something that spawned out of a conversation around a future use-case. So in short, it's not something that needs to be done today as it won't immediately solve anything. After a discussion about general usefulness of enum strings, this issue was created.

I found #1393 when I was looking for this request already, and I was concerned around the talk of beta and how it may not be easily to support this kind of feature quickly. So I logged this in the hopes to pair-down to the request I was after.

@kameshsampath
Copy link

how about just having the default existing data types and then add a spec field called choices which can list what values it need to be e.g.

  - name: colors
      description: >-
        The context directory within the source repository
      choices: 
         - blue
         - green

and if the parameter type is array then we can do do mutli from the chocies

@vdemeester ^^

@tekton-robot
Copy link
Collaborator

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.

/lifecycle stale

Send feedback to tektoncd/plumbing.

@tekton-robot tekton-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 9, 2020
@vdemeester
Copy link
Member

/remove-lifecycle stale

@tekton-robot tekton-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 10, 2020
@tekton-robot
Copy link
Collaborator

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale with a justification.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

@tekton-robot tekton-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 8, 2021
@bobcatfish
Copy link
Collaborator

Does it make sense to keep this issue open, or should we focus on our attention specifically on #1393 and tackling param types as whole?

@andrewballantyne
Copy link
Author

Agreed @bobcatfish, #1393 will be able to cover this. This was not adopted at the rate that I had hoped it would be so we can lean into #1393 as the solution that will cover this scenario as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.
Projects
None yet
Development

No branches or pull requests

6 participants