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

feat: option to ensure variable is within the list of values #1827

Merged
merged 6 commits into from
Oct 18, 2024

Conversation

vmaerten
Copy link
Member

@vmaerten vmaerten commented Sep 20, 2024

An extension of requires vars.

Added the ability check if a variable is one of allowed values. It only works for string right now.

It useful when we call a task from the CLI using a var. For example :

version: '3'
tasks:
  deploy:
    cmds:
      - echo "deploying to {{.ENV}}"
    requires:
      vars:
        - name: ENV
          enum: [dev, beta, prod]

I want to be sure that the user can only deploy to those envs.

@mgbowman
Copy link

First off, awesome idea! 💪🏻

Maybe it's just me, but allowed_values seems a bit verbose.

What do you think about values or choices or options?

@pd93
Copy link
Member

pd93 commented Sep 20, 2024

Maybe it's just me, but allowed_values seems a bit verbose.

What do you think about values or choices or options?

jsonschema uses enum for this: https://json-schema.org/understanding-json-schema/reference/enum

@vmaerten vmaerten changed the title feat: Add validation to ensure variable is within the list of allowed values feat: option to ensure variable is within the list of allowed values Sep 20, 2024
@vmaerten vmaerten marked this pull request as ready for review September 20, 2024 11:01
@vmaerten
Copy link
Member Author

I like both values and enum

@vmaerten vmaerten changed the title feat: option to ensure variable is within the list of allowed values feat: option to ensure variable is within the list of values Sep 20, 2024
@pd93
Copy link
Member

pd93 commented Sep 20, 2024

I'm fine with either. The only thing I will say in favor of enum or choices etc is that it makes it obvious that there is a constraint. values could be misinterpreted. i.e. are they allowed values? default values, will it loop over them etc.

@andreynering
Copy link
Member

Between these options, I like enum the most.

@vmaerten
Copy link
Member Author

vmaerten commented Sep 20, 2024

I've asked to chatGPT (and some friends)

If clarity is your priority, allowed_values is solid.
If you're looking for something shorter but still clear, choices or enum are great picks. Enum has the advantage of being standardized in some communities.

Naming is hard (and cannot be modified after), we need to choose it carefully.

I think I'll go for enum

EDIT: I was writing. after seeing @andreynering thought. I'll use enum

@vmaerten vmaerten merged commit a359104 into main Oct 18, 2024
14 checks passed
@vmaerten vmaerten deleted the feat/add-vars-validation branch October 18, 2024 16:16
vmaerten added a commit that referenced this pull request Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants