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

Feature Request: Support configuration as a javascript module. #4787

Closed
thomasmichaelwallace opened this issue Mar 1, 2018 · 32 comments
Closed

Comments

@thomasmichaelwallace
Copy link
Contributor

This is a Feature Proposal

Description

While YAML files are flexible for configuration thanks to the custom variable system offered by serverless and the format features like anchors; they're still not flexible enough for all use cases (for example, you cannot use anchors to form a list from multiple sources). You can see this already in some of the issues being raised (a quick search gives me #4218, #3356 and #4571 to start with).

Many other Javascript framework/programs solve this problem by allowing you to return the configuration as an object (think webpack.config.js, or mocha/karma). This gives you the whole javascript language in which to solve you particular configuration issue; and people can even provide plugins for common problems.

Configuration Changes

I suggest that serverless could also support taking configuration from a javascript file that module.exports = { ...config }.

@HyperBrain
Copy link
Member

Hi @thomasmichaelwallace . This is already possible. Just use a serverless.js instead of a serverless.yml that exports an object. This less known configuration is quite limited, because you must export an object (with the same contents as a serverless.yml) but it lets you build your configuration object dynamically with JS.
However, as already discussed in other tasks, there will be no extension of this approach but the current object export to keep Serverless' system configuration and setup under control.

@thomasmichaelwallace
Copy link
Contributor Author

Oh- that's interesting.

I'm not sure it would need extending- in fact, I'm not really following why it's limited (as it's arguably just as easy to express it as a yml then as a javascript object)?

Maybe it's worth documenting? It's definitely something I wish I'd known before handing rolling my own concatenation based approach.

@HyperBrain
Copy link
Member

The limitation is only applied to the fact that is must export an object (regardless how this is assembled within) and that it cannot have access to rudimentary configuration properties in serverless itself, because it is the source of truth of the configuration by itself and the whole framework will be setup afterwards.

That's why it is not documented and rather a somewhat "hidden" feature. It is very different than e.g. the webpack configuration, because that has access to a fully configured Serverless instance and can use that to implement fully dynamic configuration scenarios for webpack.

@thomasmichaelwallace
Copy link
Contributor Author

thomasmichaelwallace commented Mar 1, 2018

I think you might not have understood me there-

I meant how a webpack.config.js works with webpack. (not the serverless webpack plugin). In that case it doesn't seem that different at all- the webpack.config.js doesn't have access to the webpack instance it's configuring (although webpack do expose some static helper functions through their module).

It also doesn't seem a limitation when compared to yml/json, which would also not have access to the "rudimentary configuration properties in serverless itself"? Unless I'm missing something?

I've answered a fair few questions from people using the serverless framework who have experience with configuration as a module.exports = javascript object (like webpack's webpack.config.js, or eslint's @ -style extends objects, or karma's configuration...) but don't know much about yaml and it's anchoring system; or need to do something that yaml can't (e.g. merging arrays for pulling in shared IAM roles or conditional inclusion of resources). So I feel like unhiding this feature could be a real benefit.

@simonbuchan
Copy link
Contributor

Features that may or may not be supported, but should be:

  • Exporting a promise
  • Exporting default, rather than module.exports =
  • Exporting a config function that gets passed some sort of config from CLI (e.g. webpack --env) - probably ${opts} values?
  • Combinations of the above

e.g.

export default async ({ stage = 'dev' }) => {
  const vpc = await getStageVPC(stage);
  return {
    provider: { name: 'aws', ..., vpc },
    functions: {
      ...
    }
  };
};

@simonbuchan
Copy link
Contributor

It would also be nice to support multi-targeting (eg, deploy multiple services) and expose the implementation of ${file(foo.yaml)} for re-use, but those would be different issues.

@thomasmichaelwallace
Copy link
Contributor Author

I would guess that export'ing default would be pretty hard, as node still hasn't fully implemented es6 module support.

Promises are an interesting one, I expect that's resolvable on the sls side without doing too much damming... (e.g. do a typeof test on what gets returned and pass objects directly, send arguments to functions and wait on anything that returns a then'able before continuing.)

@simonbuchan
Copy link
Contributor

Ah yes, export default would (currently) need transpilation, so something like rechoir or just directly using interpret like webpack would be nice.

@erikerikson
Copy link
Member

From an ideation standpoint, it is fun to consider how powerful this could be.

Something like this has large long term consequences and there has been caution appropriately applied.

The UX of this feature is poor and documentation as you note is non-existent. If you want to improve it, no one will be able to stop you from submitting PRs. That said, I would encourage you to build your understanding of the context and concern space in order to increase the probability that your actions spend people's time efficiently, including your own.

It seems this particular issue can be closed given that the requested feature exists.

@simonbuchan
Copy link
Contributor

If you want to improve it, no one will be able to stop you from submitting PRs.

Muahaha?

That said, I would encourage you to build your understanding of the context and concern space in order to increase the probability that your actions spend people's time efficiently, including your own.

I would think that's what this issue would be for?

@erikerikson
Copy link
Member

Funny... with the title, "Feature Request: Support configuration as a javascript module." I thought it might be focused on asking for the implementation of the existing feature supporting serverless.js @simonbuchan.

@thomasmichaelwallace
Copy link
Contributor Author

You might argue that leaving it completely undocumented means that it is unsupported.

As already stated I am absolutely happy to try and solve the various problems the limitations of yaml configurations have left by using a JavaScript configuration in my own projects and feeding back via a PR to the documentation (or even a patch to the loader to support some of the functionality @simonbuchan suggested).

However both your ("build your understanding of the context and concern space in order to increase the probability that your actions spend people's time efficiently, including your own") and @hyperbrian 's comments suggest there's still a lot of discussion to be had.

Whether we do it on this issue, or as a result of a PR (although an open issue to PR against would be nice), I really don't mind.

The limitations of yaml are from my own experience clearly a thing, though- and it would be great to come up with a js config approach that was supported (documented, with extended loader for getting the required information from serverless- I'm thinking about the stage at thethe very least here) would make my experience much better- and hopefully (noting @simombuchan 's enthusiasm) someone else's too.

@simonbuchan
Copy link
Contributor

What's with the sarcasm?

I meant that it's currently possible in theory, but in practice using it immediately runs into practical issues that make it not worth using over the current documented YAML format.

Coming up with a rough list of "this is what should be supported before bothering to document this" doesn't seem unreasonable to me, even if actually implementing each of them is covered by a separate issue.

  • Everything interesting in node is async, so supporting promises is a pretty obvious win that allows doing useful things like looking up VPC config. It's also quite easy to support if the config loading is already async (I haven't checked).
  • Although the JS config could return variables like ${opt:stage} as part of the resulting config, it's pretty clear that most cases that would not be covered by YAML would want access to those while executing the config. To me this is the one that really needs discussion of what is useful, what should be supported, etc. It could simply pass the CLI opts, but is that going to block things in the future? What about passing the Serverless instance, and the config can do whatever it wants? Does that break things?

Modules / transpilation isn't required for that sure, so push that off for the initial release - node 8 supports the really useful stuff anyway. I mostly mention it because it's becoming a common feature of JS CLI config files, so it will be expected at some point, and at the time I was thinking config.default || config.

@simlu
Copy link

simlu commented Mar 13, 2018

Currently serverless.yml has some deficiencies (read bugs) that we are working around by using serverless.js and custom loading logic.

Other substantial problems with loading configuration surfaces over the past year - and looking at the code there are various workarounds implemented to solve these. In short it feels like config loading has some design problems.

I think that rather than spending time extending serverless.js logic separately, we should consider reworking the loading logic for serverless.yml and in the process exposing functionality that can then be used to improve serverless.js loading logic. Ideally the very same flow is used for js and yml configuration (e.g. for passing in a partial config to js).

Let's rework what we have first and then consider extending serverless.js. Otherwise it could get really messy?

@erikerikson
Copy link
Member

I'm definitely not arguing that we should ignore these pain points. In fact, thank you for sharing them @thomasmichaelwallace and @simlu. This feature is in a bit of a hacked state, no question. Some would have it removed while we also clearly have passionate adoption.

Fair call out @simonbuchan though I experienced your preceding response to my serious recommendations similarly. Those two points seem the most reasonable though particularly the latter can introduce some complexity and nuanced circumstances. There is particularly some partly configured weirdness that can occur and cause problems.

To be clear, my suggestion that it might be good to create a more specific and targeted issue or PR. I didn't want to totally spell this out because politics and because I have questions where I might have a more complete understanding of risks. Let me note that while I am a long term member of the community, I am not a decider with rights to approve or disapprove anything. My apparently fruitless attempts were to recommend an approach and strategy that would increase the probability of an outcome that you might regard as success.

I think being concrete and specific while also prioritized is valuable.

@erikerikson
Copy link
Member

@simlu can you expand on what you mean by "reworking the loading logic"? What do you envision?

@simonbuchan
Copy link
Contributor

simonbuchan commented Mar 14, 2018

@erikerikson Sure! I was more just confused by your earlier reply and was overly glib in my confusion 🙇‍♂️.

I've seen (and created) multiple issues that started as "I want X" turn into "Actually the real issue is that you want Y", and often it's easier to follow and less bother when the history is kept, so I was simply trying to get clarification on why using this issue wouldn't work for you. If someone made you a member, that's enough trust to make calls on that sort of thing!

Regarding the second, yeah. I could open an issue for the config function API, and a PR for supporting promises? The latter looks really simple, since it's already inside a promise, so the validation check just needs to also allow promise-likes (and add tests).

@simlu
Copy link

simlu commented Mar 14, 2018

@erikerikson The issues I refer to in particular are:

  1. sls print doesn't always produce the same output that would be used by sls deploy (oh joyful debugging)
  2. parameters are not necessarily resolved, e.g. ${opt:stage} might not be resolved even thought --stage=xxx was provided
  3. different parts of the config are loaded at different stages, and it is not clear at all what is loaded when - this causes file resolution to fail when variables are not available (even though they are)

We'll be opening bug reports with more details, however these are some deficiencies that indicate that taking a step back and looking at the config parsing and resolution logic high level might be advisable. Proper design should minimize the changes of these types of bugs occurring and provide transparency when they do.

Some questions I had while digging through the code: Is there a reason that ${file(...)} and ${opt:...} are not resolved as a very first step when loading the configuration? What are the stages that should be handled wrt config loading, at what point should what variable types be loaded and which competent needs to access the config when? Why is there a Variables.js, a yaml loader and a json ref resolution all separate?

In my opinion config resolution is important enough that it should be handled in one class transparently.
Something like: (1) obtain config and resolve as far as possible (2) when new information becomes available resolve config further and so on

Honest feedback: Our experience and struggle with config resolution has forced us to write our own custom resolution logic in serverless.js, basically replicating variable substitution and file resolution logic. We can now see what is passed to serverless and debug it easily. Ideally at some point serverless would do something similar (as a first config resolution step) and we could go back to using serverless.yml - however with the amount of issues we found so far (ours plus previous issues) there is little confidence without significant rework.

@erikerikson
Copy link
Member

erikerikson commented Mar 14, 2018

Sorry for the confusion @simonbuchan. I wrote a missive but just lost it to the ether. This will be abbreviated. I find facing a blank issue or PR drives me to clarity and through many "X, no, Y" thoughts. This can help us all with the limited time we have to invest as well as helping us arrive at a better end state. Links to previous discussions are great embeddings if the summary doesn't clarify for the reader.

@simlu 1. Should be resolved in master now. I'm not quite sure what you mean by 2. but there were a number of fails to resolve bugs fixed recently and that too is in master. If any issues continue to exist, please report them. I'm not sure quite what 3. is referring to but broadly process environment is resolved during process creation by nodejs, CLI options are processed by the framework, the serverless.* is loaded, then some content is pulled into the service object (the internal representation of the loaded config) before variable resolution occurs. I've recently contributed a significant refactoring of and increase of discipline in the variables class including the addition of test cases with user scenarios to avoid regressions. Additional cases or even just scenarios, if not fixes would be very welcome.

Regarding the ordering of variable resolution, the variable system has no preference about which variable types to resolve first. That is unless one variable contains another or a variable renders to yet another variable. In the former case the variable system has to render inner variables render first so that it knows what it should render in the outer variable, in the latter, the new variable it set aside and resolved in the next generation of variable resolutions to ensure replacement in place while allowing dependency issue detection and deadlock avoidance. The one exception to this is if your region or staged involve variables that must be rendered so that dependencies upon that configuration by the SSM, S3, et cetera variables can be rendered. This is called pre-population and it may address what you were speaking of in 2 and 3 above. This avoids entangling where do I get this configuration from the many places it could be provided from the concern of detecting and resolving variables.

You are arguing for the entanglement of file parsing and variable resolution. This would open the door to format specific bugs, a lot of additional implementation burden for existing file formats with parsers, and a lot of complexity as those two complicated concerns intermingled. It wouldn't be impossible to implement but I don't know of anyone who's volunteering to take the next large chunk of their time to implement these things for each of JSON, YAML and also JavaScript. I believe it's better to treat format parsing and variable resolution as separate steps.

I feel you on the pain of the recent implementation of the variable system that has been apart of serverless for some time now. I invested a lot of time to refactor it and set back personal goals to do so. It held back some of our teams from using the more efficient features since 1.11, so this isn't just a nicety I'm offering. Nor is my interest in specific scenarios where the behavior is not as you expect. As written there it's not enough for me to go and write unit tests and fixes that ensure your problems don't ever occur again.

Not to leave the merge consideration unmentioned but I suspect a few operators would be appropriate such as merge, also stringify, if not parse too. This is why I asked you to be specific in the previous issue about your use case and what you're trying to enable to ensure that we actually served your needs and served them for the general case in a responsible way that was reconciled with the rest of the system. Just to be concrete, the merge plug-in you recently linked doesn't handle cyclic references nor basic types such as dates that are properly handled by the variable system. Clearly that hasn't been a problem for you but for some of our users a lack of support would be a breaking change that disrupted their workflow. Not that you were suggesting that plug in was production-grade appropriate for immediate inclusion to the framework, just trying to be concrete about some of the issues involved here. More than anything just trying to be helpful.

@erikerikson
Copy link
Member

Regarding 3, maybe you meant that the variable system didn't care about ordering between variables at all. What are the results of this was that the unresolved options could be used in their unresolved form in an attempt to call SSM or the like. This is because the variable system does not have a formal notion of dependency between variables except in the cases I have already mentioned. With pre population this should be resolved.

@erikerikson
Copy link
Member

Regardless, it's probably worth you time to try master. It solves many issues in this area. There's been extensive validation already but we'd love to catch any issues earlier than later. Therefore, if you discovered any I'll be happy to prioritize their resolution.

@simonbuchan
Copy link
Contributor

@erikerikson I created the PR and issues as promised (hah! a pun!), so I'm happy to see this closed now. I'm still interested in getting transpilation done at some point, but lets get these out of the way first!

@thomasmichaelwallace
Copy link
Contributor Author

Sorry- I'm on GMT so I slept through all the discussion!

Thanks @simonbuchan (especially for the much faster turn-around with the promise PR than I would have ever achieved!) From my perspective, I see a lot of the things that serverless does to extend yaml (the variables, pulling in files &c) as a sign that maybe yaml isn't really suited for more complicated configurations (like how you can configure babel using a yml/json file until you need to do something hard, when then you can use js), so I'm especially excited to follow #4828.

@erikerikson - thanks for taking the time to expand and give some guidance. I've raised #4829 to add documentation of the .js configuration approach, which is more specifically what I would consider "support" in this case.

@simlu - interesting to hear about your experience. I would really appreciate it if you would take a look at the PR I eventually achieve through #4829 - as it sounds like you're substantially more experienced at using javascript as config for serverless than I am! I hope you don't mind but I'm going to close this issue, as I'd guess that a lot of your points will be easier to manage as separate bug reports.

Cheers for all your time!

@erikerikson
Copy link
Member

Thank you both @simonbuchan and @thomasmichaelwallace!

@simlu
Copy link

simlu commented Mar 14, 2018

PST here, so just getting caught up on the replies.

Lots of good discussion points here. Appreciate all the work you have done @erikerikson ! We'll test against current master and create bug reports as appropriate.


@erikerikson
You are arguing for the entanglement of file parsing and variable resolution. - Isn't this already the case though? You need to resolve variables to be able to resolve file dependencies and depend files can contain variables that have to be resolved. Are we talking about the same thing? Independent but alternating?

[...] implement these things for each of JSON, YAML and also JavaScript. - Not necessarily. The file loading for JSON, YAML and JavaScript should be handled completely separate (for serverless.yml, serverless.js, serverless.json or other dependent files) - and there are libraries for that. Once the content is loaded it shouldn't really matter what the format for the file was as further resolution should be identical - this is what I was referring to before.

Just to be concrete, the merge plug-in you recently linked doesn't handle cyclic references nor basic types such as dates that are properly handled by the variable system. - I should have clarified, we're not actually using that plugin since we are using custom loading logic in serverless.js. I can clarify more in that issue, but basically we are deploying two stacks: a data stack and an api stack completely separate based on --stack we provide. Then we load the correct stack definition as appropriate (the entire file).

@thomasmichaelwallace
I see a lot of the things that serverless does to extend yaml [...] as a sign that maybe yaml isn't really suited for more complicated configurations - You might have a very good point here. Problems with existing functionality, but also lack of it caused us to write our custom parsing logic. Maybe serverless.js is the way to satisfy power user needs here instead of extending on the yaml custom logic.

@erikerikson
Copy link
Member

@simlu PST here too (child had me awake early) ;P

I was making the distinction between parsing and loading. You are effectively recommending variable resolution during parsing (here, for example so that the yaml level merge operation could work on the result of the serverless variables level file resolution. Simon had also suggested similar behavior in their discussion of ASTs). Otherwise, you're describing what is already implemented. There are perhaps limitations of things the framework is unwilling accept variables in.

Once the content is loaded it shouldn't really matter what the format for the file was as further resolution should be identical

Entirely agreed

Interesting. Two services facaded by a single serverless.js. Seems a bit odd and I feel like I'm missing something. I'd just have two services. In fact, we do this regularly.

@simonbuchan
Copy link
Contributor

Simon had also suggested similar behavior in their discussion of ASTs).

  1. I think you can assume "his" is ok, given my name, but thanks for the consideration 😉
  2. Actually, just a possible alternative syntax that wouldn't conflict with any existing values and would reduce parser edge cases - e.g. it would parse !file foo.js as something like new FileReference("foo.js"), and the variable resolution step would know how to unpack it. I don't know if YAML tags can nest though....

@simonbuchan
Copy link
Contributor

Interesting. Two services facaded by a single serverless.js. Seems a bit odd and I feel like I'm missing something. I'd just have two services. In fact, we do this regularly.

This makes more sense if you're sharing a lot of code. It's also a simpler deployment story.

@erikerikson
Copy link
Member

@simonbuchan 1. 👍 Thanks for informing. 2. still implies entanglement. Entanglement can be worth the cost but I'm not convinced it's warranted here. I'm not sure how this would work for JSON or JavaScript though I guess it could be a yaml only feature. Sounds unexciting to me for what little that might be worth.

I think maybe it's worth noting that the serverless configuration is run through validation by Service.js prior to variable resolution. This means there are off-limits portions of the configuration with out any means if discovering this or a reason to expect this. Unless I am remembering incorrectly. I should find extra time in the day to read this code more, maybe? If this want the case you could have your way with the config, including a shared file to specify provider and the like and remain DRY. Does this sound like it might get at the root issue? I guess I think of serverless.js as a mechanism to support generation of config off a custom meta-specification. This would usually create least privilege related risks but I can see the temptation to do it. The lack of rights individuation is a whole'nother long standing beast though.

@erikerikson
Copy link
Member

Not to ignore the ability to npm i <local dir>. The set up you describe, if I understand it properly, shares everything rather than the specified shared assets.

@simlu
Copy link

simlu commented Mar 16, 2018

@erikerikson We've tested against current master and a lot of issues were resolved! Great work!

The only issue that is still outstanding so far is: #4837

@erikerikson
Copy link
Member

Awesome, thank you very much @simlu. I'll take a look at the investment required to resolve #4837. Please let us know if anything else pops up!

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

No branches or pull requests

5 participants