-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
Feature Request: Using both external and serverless.yml env vars #4571
Comments
Thank you for reporting @simlu 👍 |
Is there a way to achieve this by using different contexts? E.g. you could use function + service environment variables. Is there a more generic way? |
@horike37 Would you mind pointing me to the relevant file(s)? I'd like to make this my first contribution! Cheers |
AWESOME @simlu 👍 💯 It would be great if you can implement this improvement! In general, the yaml file is parsed within YamlParser.js, which actually leverage js-yaml as a YAML parser library. Therefore, I think the base way is that If not, we should consider other solution for that. |
js-yaml is currently invoked with "safeLoad". This is safe but disallows YAML references that load multiple files as far as I know. |
@horike37 Thank you for the input! |
Seems like https://github.com/whitlockjc/json-refs is used to resolve the dependencies after the yaml is parsed. It makes sense that we are seeing this error. Any objections to restructuring it so that the resolve of dependencies happens before parsing the yaml? |
@erikerikson , do you have any opinion here regarding the json-refs location? It might work only partially if resolved before parsing the yaml. |
Sorry, no opinion in particular. It's not clear to me that json-ref will deal with unparsed yaml. That module was originally introduced in order to help deal with circular references in the service object (i.e. It seems there is some confusion here though. Variable resolution happens at a whole different phase than yaml parsing but @simlu, you're trying to mix the two here. The yaml is parsed into a JavaScript object and only after that parsing is complete are any variable populations attempted. Therefore as written, you are not attempting to merge an object but a string (i.e. Adding a merge capability into the variable system doesn't seem unreasonable so you could accomplish this but it seems that it would be easy to accomplish the same result now albeit less tersely using overrides on individual values. Alternatively, unless you're actually merging over top of other values, you could simply be more explicit about where the data is coming from. |
@erikerikson Ah yes. Sorry I wasn't clear at all. There are three stages; Basically the syntax we want to support works fine for (1). However it leaves the data invalid for (2). So my suggestion it to move (3) before (2) or even before (1). From what you are saying it sounds like that defeats the purpose of (2) though? Do you have a better idea? I can always hack something in for this one case. But ideally we solve this properly :) |
I won't be responding fully just now (I'm on vacation) but I think it would be helpful to be absolutely crisp about the use case you're trying to enable. Is it simply the merge? Is there something you are blocked from accomplishing? Et cetera... |
Following up here...
Due to this and no plan to adjust the ordering because of the complexity of properly altering YAML (consider arbitrary indentation contexts when inserting a complex object), I think that this is asking for a new variable capability. Which is why I recommended that if you want change you ought to be very crisp about the use case and exactly the scenario(s) you want to enable, how you would expect it to work, et cetera. FWIW, I did actually encounter a case where a merge could have been helpful, so it's not entirely insane. I'm fixing #4687 currently and will have familiarity if you can provide a legible notion of the feature request. |
In order to support the expected syntax, you would probably have to parse the config file using It might be simpler to consider permitting yaml tags for variables, similar to the cloudformation shorthands, for example: environment:
STAGE: !self provider.stage
REGION: !self provider.region
<<: !file ['config/', !self provider.stage, '.yml']
|
@simonbuchan thanks for sharing your knowledge! Interesting option that I was ignorant of but find interesting. This could imply ceding some UX control and potentially break custom formats or the like but maybe I'm just exposing more ignorance: interesting nonetheless. |
On the plus side, the use-case for custom formats was a conflict with cloudformation |
BTW, a work around here is to move your STAGE and REGION into the
|
@thomasmichaelwallace provided an interesting option elsewhere:
Particularly if you were to do something like define stage as an environment variable:
|
Although I do use this in production, it's a bit of a hack. Can I suggest we take a leaf out of webpack's book and support serverless.config.js, which exports a common is config object. That effectively allows you to do anything, as you're free to code various solutions- and use common plugins. Assuming I haven't missed that this already exists, and people think it could be a good idea, I'm happy to put together a PR. |
Well you might be able to do something like: .realconfig: &realconfig
${file(./serverless.js)}
<<: *realconfig But you're probably better off just using |
Interesting- I don't think that approach would have ever occured to me. The splatting bits works up to a point (that's what we do with the concat; as the common file is centralised); but yaml has it's limits (like no list merging, or conditional anchor references). I'm still fairly taken with the idea of webpack-like configuration, so I've raised a separate issue (#4787) as to not clog up this commentary. |
The plugin serverless-merge-config should resolve this. Big thanks to @Omicron7 - However there seem to be other problems with parameter resolutions. Considering how important config resolution is and how many critical issues surfaced around it, it might be worth taking another high level look at how the different resolution steps work and what is resolved where. After digging through the code it feels like there are a bunch of patches implemented, rather than having a high level design that solves the problems more generically. But unfortunately I didn't get super deep into untangling. References: |
I feel doing the at the AST level is definitely the right way to go, it's just it's rather a pain to do at the moment with the APIs available. |
I discussed the difficulties that the use of yaml was bringing in compared to other frameworks I also use to deliver apps (specifically I had webpack's in mind).
I then learned that serverless does actually support having a configuration exported as a JavaScript module (_exactly_ like webpack), it's just undocumented and seemingly considered too technical to ever be preferred.
Needless to say, though, a lot of these issues go away when you can write your configuration as code- npm is lousy with modules; including ones to load .env files &c.
You can see my issue and the discussion here: #4787
I'm currently in the process of upgrading the various hacks I implemented in yaml to webpack-like js configs. Once I'm done I'll try and write a blog about the experience and any pitfalls I discover.
|
@simonbuchan @thomasmichaelwallace Might be interesting for reference: #4817 (comment) Edit: Here is the important file https://github.com/simlu/lambda-serverless-api/blob/master/lib/util/yaml.js |
Based on discussion with @erikerikson I'm detailing our use case a bit more. We have two separate stacks that we deploy with serverless: One We define our Ideally we would share common information in the Discussion in the other thread would indicate that our use case is not really what serverless configuration is designed for - hence using provider:
name: aws
runtime: nodejs6.10
.stack: &stack
${file(./stacks/${opt:stack, 'api'}.yml)}
<<: *stack but that won't work for various reasons. |
We are now using yaml-boost and it works really well. Basically it replaces |
@simlu are you still using |
@iDVB Yes, we are still using I will take a look at the ticket you opened in |
That worked for me, thanks! serverless.yml
environment.yml
that allows to pass region/stage through cli:
|
Hi, back to the title. any plan to support using both external and serverless.yml env vars? |
@extJo maybe I'm missing something but try looking into the |
@erikerikson i know that way. the point is that plan to support merging the object in the referenced file into the object at the same level. |
Thanks, I was confused about you language and wanted to be sure. Glad you were aware. |
@erikerikson Sorry about that :). |
I'm not nearly the regular contributor to the framework I once was. I'd love to be but anyway... I doubt the maintainers would turn down a good PR. ;D |
The thing I need is a yaml anchor attached to the top of an imported file object. I understand the complexity with phases and varying syntaxes, but this should not still be intractable now three years later! FAILSThese are non-working examples, but represent the 3 parts that need to fit together: environment: &environment ${file(config/${self:provider.stage}.yml) environment: &environment
<<: ${file(config/${self:provider.stage}.yml) environment:
<<: &environment
${file(config/${self:provider.stage}.yml) IDEASIMO, it would be sufficient to allow a workaround syntax: environment: ${anchored_file(config/${self:provider.stage}.yml, environment)} AFAICT, This would reduce the number of different tools in play and put the control in the hands of the implementer of More atomically, one would really only need to produce a serverless function environment: ${anchor(file(config/${self:provider.stage}.yml), environment} Which then might see reuse with other config retrieving functions besides Obviously, this still conflicts with the presumption that YAML parsing is complete before any serverless function is allowed to execute. |
Would it be possible to be able to do something like this:
I have a number of functions and I don't want all of them to get db config/redis details. At the moment I'm redefining each variable individually in the lambda environments, it would be useful to be able to define different groups of configs in custom and include groups depending on the requirement. |
I've been reading the history of this issue and it seems there are 3 different root use cases:
About 1, we've opened #10264 (Stage-specific configuration) to address this. Let us know if that would help! |
Perhaps my case is a different than 2? I'm trying to mix both envs from AWS Secrets Manager and locally defined via yml (or whatever, really). |
We solved this now by compiling the yml into a single file automatically before every deploy. This is relatively easy to do and has the added benefit that you know exactly what you're deploying |
I thought about that but could not quickly find any docs on how to deploy on top of a static yml config file. const AWS = require('aws-sdk')
const package = require('./package.json')
const SSM_NAME_MAP = {
staging: 'secretname',
prod: 'othersecretname',
}
const getVersionOrHash = (stage) => {
if (stage === 'staging' && process.env.CI) {
return process.env.CIRCLE_SHA1
}
return package.version
}
module.exports = ({ options, resolveVariable }) =>
new Promise(async (resolve, reject) => {
const stage = await resolveVariable('sls:stage')
const region = await resolveVariable('opt:region, self:provider.region')
new AWS.SecretsManager({ region }).getSecretValue(
{ SecretId: SSM_NAME_MAP[stage] },
(err, { SecretString }) => {
if (err) {
return reject(err)
}
resolve({
...JSON.parse(SecretString),
VERSION: getVersionOrHash(stage),
})
},
)
}) and then on serverless.yml environment: ${file(./envs.js)} |
Definitely in the docs, i.e. |
I'm trying to use external variables from an external configuration file and internal variables from serverless.yml as environment variables.
I've tried
However this gives me the error
YAMLException: cannot merge mappings; the provided source object is unacceptable
Is there currently a way to achieve this or is this a feature request?
Note from maintainers:
You might be interested in Stage Parameters.
Related (also unanswered questions):
The text was updated successfully, but these errors were encountered: