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 passing variables via CLI #9817

Closed
pgrzesik opened this issue Aug 9, 2021 · 24 comments
Closed

Support passing variables via CLI #9817

pgrzesik opened this issue Aug 9, 2021 · 24 comments

Comments

@pgrzesik
Copy link
Contributor

pgrzesik commented Aug 9, 2021

Background

When introducing CLI options schema, we made a decision to deprecate/remove support for custom CLI options and recommended to replace their use with envrionment variables. However, it seems like environment variables can be problematic e.g. for PowerShell users (#9371 (comment)). Given that, we would like to consider adding a mechanism that would allow passing custom variables via CLI options.

Potential solutions - passing variables via CLI

Pass custom variables with single CLI option

One potential option to pass variables would be to use a single CLI option, e.g. --variables, in form like below:

serverless deploy --variables "first=1,second=2"

The potential problem with this approach is that the separator might introduce parsing errors if such separator should also be a part of passed variable's value.

Pass custom variables with multiple CLI options

Another potential solution would be to pass each variable separately with --variable flag, in form like below:

serverless deploy --variable "first=1" --variable "second=2"

This solution does not have the problem of single CLI option, but is a bit more verbose.

Accessing variables in the configuration

Separate variable source

When it comes to accessing these variables as a part of configuration file, I believe the most reasonable solution would be to access them via new variable source, e.g. cli or clivars. It could look like this:

service: example-function
provider:
  name: aws
  environment:
  VAR_FROM_CUSTOM_CLI_VARIABLE: ${cli:first}
  OTHER_VAR_FROM_CUSTOM_CLI_VARIABLE: ${cli:second}

The potential downside here is that cli or clivars names are very similar to existing opt source, which might cause confusion.

Populate values into options

Alternative approach would be to populate options that are accessible via existing opt variable sources. The issue with this approach that it might introduce name clashes and does not seem "clean" as it will no longer have only explicit options as directly passed via CLI.

Summary

In my opinion, the combination of passing such variables with multiple CLI options and having separate variable source such as cli would be the best way to move forward with this initiative, however, all feedback is welcome 🙇 @medikoo & @mnapoli - I would also love to hear your opinion on the listed approaches.

Notes:

Multiple variable options approach has been inspired by terraform -var flag for CLI variables (https://www.terraform.io/docs/language/values/variables.html)

@mnapoli
Copy link
Contributor

mnapoli commented Aug 9, 2021

If I understand correctly, the problem is for Powershell users (worsened developer experience due to environment variables being hard to use in that shell).

Since we are talking about significant changes, it might be worth considering how many users are actually impacted. Do we have a way to get an estimate of the percentage of users who face this issue?

So far what I have: 4% of users use extra CLI options to pass variables (i.e. they get the UNSUPPORTED_CLI_OPTIONS deprecation). Out of those, do we have any way to know if PowerShell users represent a significant portion of them? Without more data, I would honestly estimate that to be a very little share of those 4%, which means that only a very slim portion of users would actually be impacted. Based on that, I would honestly wait for more feedback/demand from the community before implementing a new variable system.

@lehmat
Copy link

lehmat commented Aug 9, 2021

Nice to know this was taken into consideration. Really appreciate it 👍

I like the option of being able to pass either --variable or --variables. One thing to notice is that there might be cases where variable contains both " and ' making it necessary to have quoting option (even more with --variables since comma can be present in a variable more commonly). It would be good practice to add tests to quoting and documenting how to quote the variables.

@pgrzesik
Copy link
Contributor Author

pgrzesik commented Aug 9, 2021

If I understand correctly, the problem is for Powershell users (worsened developer experience due to environment variables being hard to use in that shell).

That is correct @mnapoli - I believe there also might be issues where given env var is already used, but that can be circumvented by just using different variable name.

Based on that, I would honestly wait for more feedback/demand from the community before implementing a new variable system.

These are very good points - I believe not a lot of people are impacted and I fully agree that we should wait for more feedback and interest from community. I've created this issue for feedback/discussion purposes mostly 👍

@medikoo
Copy link
Contributor

medikoo commented Aug 10, 2021

@pgrzesik great proposal! My feedback:

  1. As you suggest I would go with multiple options, as I think there's no friendly way to gather multiple variables with single input, also with multiple options we take inspiration from Terraform, which we can consider as battle tested.
  2. I propose to name CLI param --var not --variable. It feels more concise
  3. I would name variable source user, userVar or var (although last one is probably too ambiguous for variable system). Main reason is that we're going into direction where Serverless commands could be operated purely programmatically, and then input options would not be taken from a CLI. Therefore it'll be good to settle on more generic name (which fits both: case where vars are input via CLI params and where they're input via function args), I would also definitely not mix it with opt (as that will introduce potential collisions - which imo would be a design bug)
  4. Let's restrict var key format to [a-zA-Z0-9]+, this will ensure we can easily parse the var value which can then be any string (that follows after [a-zA-Z0-9]+=)

One thing to notice is that there might be cases where variable contains both " and ' making it necessary to have quoting option (even more with --variables since comma can be present in a variable more commonly). It would be good practice to add tests to quoting and documenting how to quote the variables.

@lehmat it's a good point, still this is rather specific to terminal (and differentiates between them), so I'm not sure weather it really belongs to our docs. Still e.g AWS documents how to quote vars and we may consider to do same.

@lehmat
Copy link

lehmat commented Aug 10, 2021

Let's restrict var key format to [a-zA-Z0-9]+, this will ensure we can easily parse the var value which can then be any string (that follows after [a-zA-Z0-9]+=)

Just a thought: what about adding - and _ in the cli variable to support other than camelCase? With just allowing those that were mentioned, you would force people to write camelCase.

It is different topic should you use only camelCase since AWS supports that in resources names etc, but letting people choose would give some freedom if it does not bring any pain on implementation side?

@medikoo
Copy link
Contributor

medikoo commented Aug 10, 2021

Just a thought: what about adding - and _ in the cli variable to support other than camelCase?

@lehmat that's a good point. I've initially proposed to restrict it for two reasons (1) settle on one naming convention (2) follow convention of JS variables.

But I don't have a strong opinion on that, also I'm aware that Serverless Framework is not used just by JS devs and in CLI environment it's more natural to use hyphenated strings, not camel case, so we definitely can relax that.

@pgrzesik
Copy link
Contributor Author

Thanks @medikoo 🙇

As you suggest I would go with multiple options, as I think there's no friendly way to gather multiple variables with single input, also with multiple options we take inspiration from Terraform, which we can consider as battle tested.

👍

I propose to name CLI param --var not --variable. It feels more concise

Sounds good to me 👍

I would name variable source user, userVar or var (although last one is probably too ambiguous for variable system). Main reason is that we're going into direction where Serverless commands could be operated purely programmatically, and then input options would not be taken from a CLI. Therefore it'll be good to settle on more generic name (which fits both: case where vars are input via CLI params and where they're input via function args), I would also definitely not mix it with opt (as that will introduce potential collisions - which imo would be a design bug)

I was thinking about var source as well, but as you mention as well, it felt a little bit confusing to have var(iable) be the variable source. I was also thinking about customCliVar or something along those lines, but it sounds a bit too verbose I believe.

Let's restrict var key format to [a-zA-Z0-9]+, this will ensure we can easily parse the var value which can then be any string (that follows after [a-zA-Z0-9]+=)

Sounds good to me, along with @lehmat proposal to also support _ and - - I believe there is not harm in supporting that convention and some people might be more used to it.

@medikoo
Copy link
Contributor

medikoo commented Aug 10, 2021

I was thinking about var source as well, but as you mention as well, it felt a little bit confusing to have var(iable) be the variable source. I was also thinking about customCliVar or something along those lines, but it sounds a bit too verbose I believe.

Maybe then name it a user, as technically a user input is here source of a data (?)

Sounds good to me, along with @lehmat proposal to also support _ and - - I believe there is not harm in supporting that convention and some people might be more used to it.

Totally 👍

@dls314
Copy link

dls314 commented Jan 24, 2022

Is there any update / resolution to this? I see that the https://www.serverless.com/framework/docs/providers/aws/guide/variables#referencing-cli-options documentation still exists and doesn't mention the deprecation as from #9371

@mnapoli
Copy link
Contributor

mnapoli commented Jan 25, 2022

@dls314 it is still possible to reference CLI options that are supported/documented (e.g. --stage, --region, etc.). That's why the variable you linked to still exists.

By the way, on the back of my head I was thinking this feature could integrate pretty naturally with parameters (${param:xxx}):

serverless deploy --param=foo

This would set (and possibly override) any foo parameter. This would also make Serverless Framework parameters consistent with CloudFormation/SAM/CDK parameters (e.g. https://docs.aws.amazon.com/cdk/v2/guide/parameters.html) which can be provided in the CLI too.

WDYT?

@dls314
Copy link

dls314 commented Jan 25, 2022

@dls314 it is still possible to reference CLI options that are supported/documented (e.g. --stage, --region, etc.). That's why the variable you linked to still exists.

I see now; I saw ${opt: and ignored that it referenced the stage instead of a custom name.

@dls314
Copy link

dls314 commented Jan 25, 2022

I'm making use of the ${opt: syntax extensively across a number of serverless projects and their associated build, test, and deployment automation, and any move to another syntax is going to be enough cost that I probably can't migrate beyond the last version where it's supported.

The alternative here for me is that I can spend the effort migrating for currency in the serverless framework (when it is unavoidably necessary), or I can spend that effort migrating IaC to CDK and minimizing the serverless framework footprint.

@coyoteecd
Copy link
Contributor

The original reason was that you want to prevent typos. Wouldn't that be solvable by declaring a list of known cli options in the serverless.yml file?

I am using serverless on Windows (dev) an Linux (CI) and using env vars sucks. I've considered adding cross-env as dep, or even building my own plugin that extends the cli with options and re-exposes them as a variable source.

@pgrzesik
Copy link
Contributor Author

pgrzesik commented Feb 16, 2022

@dls314 it is still possible to reference CLI options that are supported/documented (e.g. --stage, --region, etc.). That's why the variable you linked to still exists.

By the way, on the back of my head I was thinking this feature could integrate pretty naturally with parameters (${param:xxx}):

serverless deploy --param=foo

This would set (and possibly override) any foo parameter. This would also make Serverless Framework parameters consistent with CloudFormation/SAM/CDK parameters (e.g. docs.aws.amazon.com/cdk/v2/guide/parameters.html) which can be provided in the CLI too.

WDYT?

I think it makes a lot of sense, however, the actual use would be something like this:

serverless deploy --param="foo=value"

as we want to assign both key and value at the same time.

@mnapoli Does that sound/look good to you?

@medikoo What do you think about scoping these under param variable source?

@dls314
Copy link

dls314 commented Feb 16, 2022

I think it makes a lot of sense, however, the actual use would be something like this:

serverless deploy --param="foo=value"

Currently this uses the ${opt: syntax in serverless.yml -- could that be preserved there as well as on the command line with something like serverless deploy --opt="foo=value"?

@pgrzesik
Copy link
Contributor Author

I think that would be a bit problematic @dls314 - currently opt already has all the defined CLI options and that would introduce a possibility to clash between the values passed directly e.g. --stage dev vs --param="stage=staging" as these would translate locally to the same thing. I think it would be a better call to expose them under param variable source or a totally new variable source.

@mnapoli
Copy link
Contributor

mnapoli commented Feb 16, 2022

--param="foo=value" looks good. A few things I noted:

  • users should be able to pass the same option multiple times, to pass multiple parameters
  • allow dots (foo.bar=value) in parameters (cf. the other issue about object values)
  • --param takes the highest priority when resolving values (compared to the other options: Dashboard & stage params) -> let me know if you see reasons to take another route on this

@pgrzesik
Copy link
Contributor Author

Please note my answers below @mnapoli and let me know what do you think

users should be able to pass the same option multiple times, to pass multiple parameters

If I understand correctly, you mean something like that:

serverless deploy --param="foo=val" --param="bar=val2"

if that's the case then yes, user will be able to pass multiple parameters this way.

allow dots (foo.bar=value) in parameters (cf. the other issue about object values)

I don't think we should support something like that - I think we should support flat <key>=<value> syntax without any nesting and disallow . in key name - previously we discussed key format like this - [a-zA-Z0-9-_] and I think it's a good limitation. It also keeps compatibility with the Dashboard params at the moment. What do you think?

--param takes the highest priority when resolving values (compared to the other options: Dashboard & stage params) -> let me know if you see reasons to take another route on this

I think it should work this way as params passed via CLI should be used as "overrides" essentially.

@mnapoli
Copy link
Contributor

mnapoli commented Feb 16, 2022

I don't think we should support something like that - I think we should support flat = syntax without any nesting and disallow . in key name - previously we discussed key format like this - [a-zA-Z0-9-_] and I think it's a good limitation. It also keeps compatibility with the Dashboard params at the moment. What do you think?

Sounds good, it's future-proof anyway. If we decide to relax that later we always have the option 👍

LGTM!

@medikoo
Copy link
Contributor

medikoo commented Feb 17, 2022

`serverless deploy --param="foo=value"

Sounds like a really good idea to me.

Concerning multiple input, settling on --param="foo=val" --param="bar=val2" feels right (in description we propose something as --params="foo=val,bar=val2" but that approach would be more limited)

It also definitely should override eventual config and dashboard settings.

@pgrzesik
Copy link
Contributor Author

Hello everyone, this has now been released as a part of 3.3.0 Framework release

vicary pushed a commit to serverless-heaven/serverless-webpack that referenced this issue Mar 24, 2022
The newly added param value is typed as a string array, see serverless/serverless#9817
@frjtrifork
Copy link

frjtrifork commented Sep 7, 2022

I probably missed it somewhere.

Does using --param require using Serverless Dashboard?

The parameters documentation (https://www.serverless.com/framework/docs/guides/parameters) is not that explicit about this requirement, it mentions that you can create the params in either serverless.yml or in Dashboard but does not say explicitly that you have to use Dashboard.

But if I add something like this in the custom: section of serverless.yml:

someParam: ${param:foo, 'defaultValue'}

and then does a print:

npx sls print --stage dev --param='foo=myValue'

i get this error:

 
  "Cannot resolve configuration: "param" and "output" variables can only be used in services deployed with Serverless Dashboard (with "org" setting configured)```

If it is a requirement, I think it would be nice to mention it in the documentation.

@medikoo
Copy link
Contributor

medikoo commented Sep 7, 2022

@frjtrifork It doesn't require a dashboard. If npx sls print fails for you that way, please open a bug report providing a reproduction case (as minimal as possible)

@frjtrifork
Copy link

@frjtrifork It doesn't require a dashboard. If npx sls print fails for you that way please open a bug report providing a reprodiction case (as minimal as possible)

Thank you for getting back so quickly.

It was an error on my part. I had two different serverless projects and the one I got the error about "can only be used in services deployed with Serverless Dashboard" the serverless version had been pinned to 2.72.3.

In the other repository I have, I use version 3.22.0 of serverless and I get no error when using the ${param:foo, 'defaultValue'}

HanbyulJo added a commit to HanbyulJo/serverless-webpack that referenced this issue Nov 28, 2022
The newly added param value is typed as a string array, see serverless/serverless#9817
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants