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

Add support of serverless.js configuration file #4590

Merged
merged 5 commits into from Dec 22, 2017

Conversation

pavelvlasov
Copy link
Contributor

@pavelvlasov pavelvlasov commented Dec 19, 2017

What did you implement:

Closes #3269

How did you implement it:

Uses require to load js configuration file (expects config exported).
Added exception for plugin install command to log warning instead of modifying existing config file.

How can we verify it:

Changes are fully covered and tested with on my existing lambdas.

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: Yes
Is it a breaking change?: NO

Copy link
Member

@HyperBrain HyperBrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR 😄 . I did a review, and there should be some robustness changes.
In general, I think that the require of the js file without any checks must be changed too. My point is, that if the js file exports a function instead of a plain object, it will not be detected right now, but fail somewhere afterwards.
We should explicitly check the required JS file, that its export fulfills _.isPlainObject().
@pmuens @horike37 Do you see any generic issues with having the possibility to load a serverless.js file as done here?

} else if (exists.js) {
// use require to load serverless.js
// eslint-disable-next-line global-require
return require(jsPath);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

require can throw synchronously. You're in a promise chain here, so you should catch any error and return a promise rejection in case it fails and you must return a resolved promise if it suceeds.

return BbPromise.try(() => require(jsPath));

@@ -61,65 +62,79 @@ class Service {
serviceFilenames[serviceFileIndex] :
_.first(serviceFilenames);

if (serviceFilename === 'serverless.js') {
return new BbPromise((resolve) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not use the new Promise anti-pattern! Additionally require can throw synchronously, so it has to be catched.

return BbPromise.try(() => that.loadServiceFileParam(serviceFilename, require(serviceFilePath)));

that.package.include = serverlessFile.package.include;
that.package.excludeDevDependencies = serverlessFile.package.excludeDevDependencies;
}
if (typeof serverlessFile.provider !== 'object') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should consistently use lodash:

if (!_.isObject(serverlessFile.provider)) ...

@pavelvlasov
Copy link
Contributor Author

thanks for the feedback @HyperBrain, totally makes sense. I added checks for plain object and addressed your comments.

@horike37
Copy link
Member

Thank you for your efforting @pavelvlasov 👍

The code looks good from my perspective.
I will test this PR to check not to introduce any side effect.

Copy link
Member

@horike37 horike37 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, just commented the result of my review. Please check @pavelvlasov 😄

I tested locally but saw to fail with the following error.

[horike@ip-192-168-0-2 sls-test]$../8bites/bin/serverless package
 
  Syntax Error -------------------------------------------
 
  Unexpected token :
 
     For debugging logs, run again after setting the "SLS_DEBUG=*" environment variable.
 
  Stack Trace --------------------------------------------
 
SyntaxError: Unexpected token :
/Users/horike/src/serverless-works/sls-test/serverless.js:2
  "service": "ppppppp",
           ^

SyntaxError: Unexpected token :
    at createScript (vm.js:80:10)
    at Object.runInThisContext (vm.js:139:10)
    at Module._compile (module.js:576:28)
    at Object.Module._extensions..js (module.js:623:10)
    at Module.load (module.js:531:32)
    at tryModuleLoad (module.js:494:12)
    at Function.Module._load (module.js:486:3)
    at Module.require (module.js:556:17)
    at require (internal/module.js:11:18)
    at BbPromise.try (/Users/horike/src/serverless-works/8bites/lib/classes/Service.js:69:24)
    at tryCatcher (/Users/horike/src/serverless-works/8bites/node_modules/bluebird/js/release/util.js:16:23)
    at Function.Promise.attempt.Promise.try (/Users/horike/src/serverless-works/8bites/node_modules/bluebird/js/release/method.js:39:29)
    at Service.load (/Users/horike/src/serverless-works/8bites/lib/classes/Service.js:66:27)
    at Serverless.init (/Users/horike/src/serverless-works/8bites/lib/Serverless.js:64:25)

Here is serverless.js I'm using

{
  "service": "ppppppp",
  "provider": {
    "name": "aws",
    "runtime": "nodejs6.10"
  },
  "functions": {
    "hello": {
      "handler": "handler.hello"
    }
  }
}

Any ideas why?

@@ -109,6 +109,14 @@ class PluginInstall {

addPluginToServerlessFile() {
return this.getServerlessFilePath().then(serverlessFilePath => {
if (_.last(_.split(serverlessFilePath, '.')) === 'js') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need the same process in plugin/uninstall/uninstall.js as well. Otherwise, a plugin can't be removed from serverless.js when running sls plugin uninstall

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, added log message for uninstall as well 👍

@@ -357,6 +357,24 @@ describe('PluginInstall', () => {
});
});
});

it('should not modify serverless .js file', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here.

@HyperBrain
Copy link
Member

HyperBrain commented Dec 21, 2017

@horike37 the serverless.js should do a module.exports = { ... }; and not just contain a JSON body

if (jsonExists) {
return serverlessJsonFilePath;
}
return serverlessJsFilePath;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we check here for the existence of the JS file too and reject with an error as last resort?

return fileExists(serverlessJsFilePath).then(jsExists => {
  if (jsExists) {
    return serverlessJsFilePath;
  }
  return BbPromise.reject(new this.serverless.classes.Error('Could not find any serverless service definition file.'));
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored this method a bit, looks much nicer now.

@horike37
Copy link
Member

@HyperBrain
thanks for following up. it works 👍

@@ -109,6 +109,14 @@ class PluginInstall {

addPluginToServerlessFile() {
return this.getServerlessFilePath().then(serverlessFilePath => {
if (_.last(_.split(serverlessFilePath, '.')) === 'js') {
this.serverless.cli.log(`
Can't automatically add plugin into "serverless.js" file.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why cannot a plugin be installed automatically?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the js file only exports the serverless object, but hides the details how that is kept or generated. So the plugin plugin does not have any chance to tell the serverless.js to include the new plugin into the exported object the next time it is loaded.
In theory the whole service definition could be dynamically compiled depending on some environment settings. That way we have to treat the serverless.js as opaque and the exposed object as read-only.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I see 😄

@pavelvlasov
Copy link
Contributor Author

@HyperBrain @horike37 looks like there're couple of flaky tests. Will appreciate if you trigger another build on this PR.

@horike37
Copy link
Member

horike37 commented Dec 21, 2017

@pavelvlasov
thank you very much for your updates 👍

Will appreciate if you trigger another build on this PR.

done the restart and see passing the test.
Are you ready review again?

@horike37 horike37 added this to the 1.26 milestone Dec 21, 2017
@pavelvlasov
Copy link
Contributor Author

@horike37 thanks! yes, should be good to go

@horike37
Copy link
Member

@pavelvlasov
Great to see working fine as expected 💯
everything is OK! merging...

@crrobinson14
Copy link

I have a question about the approach this change is meant to implement, before this ticket is closed (or should it be a follow up?).

It seems very attractive - we can now finally do things like auto-discovering functions defined in subfolders and building the config for them dynamically. That's great! But it seems like it's missing an important detail that would put it over the top: there's no context passed in when the script is called.

That means when you want to do something like variables, you still do this unless I'm missing something:

domain: 'lambda-${self:provider.stage}.mydomain.com',

It would be nice if things like the stage and other context items were passed into the script, or how to access them was documented somewhere. Otherwise developers like me are going to be poking around looking for globals to access to get these items in undocumented ways.

Alternatively, if the statement is that the developer is EXPECTED to export them as-is like the above example, and Serverless will fill them in "later", I feel it's going to create confusion because they look an awful lot like JS template strings. Most standard ESLint rule sets (e.g. AirBNB "base") are going to complain about this.

I feel like before this is finally closed out, a decision should be made, and documented/communicated to developers, on how they are "supposed to" deal with variables in these configs. Replace then export? Or export and let Serverless replace?

@HyperBrain
Copy link
Member

HyperBrain commented Feb 1, 2018

Imo, the serverless.js variant to expose a serverless configuration is just a different way to compose a specified serverless configuration. The js must return a valid serverless configuration. It has no other additional features, but letting create the config dynamically.

It is just a third way to specify the config after serverless.yml and serverless.json. Having special inputs just for this case would change the core approach of serverless and would have side effects on the other variants too. The delivered contents must exactly match the standard serverless configuration schema, because that's also the base the whole framework stands upon as well as all the plugins for the framework.

That includes variable resolution. The script can return variables in the standard variable notation and serverless will resolve them.

@crrobinson14
Copy link

That makes 100% sense, @HyperBrain but I'm a little confused about the final meaning here. I'm not advocating for changing the format in any way. If I can restate the question, it would be "is the goal to have the emitted config object contain "final" strings, or "strings that contain still-to-be-replaced variables?"

If so, it would be great to document this clearly because I think a lot of folks are going to have ESLint rules that complain about these strings, and will want an eslint-disable for these files. Messy, but easy.

Otherwise, it would be extremely helpful to pass in (as gulp/grunt/other build tools do) or globally expose some type of object that at least contains stage (there may be other good-to-know items but that's the only one I see an immediate use-case for...) The bulk of these little replacements is based on that one little detail. It could dramatically simplify/offload how much Serverless does internally for variable replacement if it can rely on the JS file to make those decisions and just return the final strings...

I suppose a simple enhancement that would achieve this (I'd be happy to submit a PR) would be to detect if the export is a plain object or a function. If it's a function, call it with some type of simple context object that contains values such as stage.

@pavelvlasov
Copy link
Contributor Author

totally agree with @HyperBrain.
Framework read the config first and then expose variables and it would be pretty hard to change that behaviour without deep rework.
But you still have access to cli arguments in process.argv, e.g.

const program = require('commander');

program
    .option('--stage <n>', 'stage')
    .option('--region <n>', 'region')
    .option('--bucket <n>', 'bucket')
    .parse(process.argv);

process.write(program.bucket);

at least you will be able to do some preprocessing depending on stage.

@crrobinson14
Copy link

Interesting. I hadn't considered accessing stage from the command line args directly. Is that a formal recommendation or just brainstorming? It seems like something that could have some side effects / issues, e.g. if the way stage is determined changes (i.e. some future feature allows stage to be determined in a way other than the command line)...

@crrobinson14
Copy link

crrobinson14 commented Feb 1, 2018

Just to keep this small/focused, after a bit more thinking the only items I could REALLY make a use case for here ARE the things controlled by command line args. IE, nothing that comes later in the processing workflow.

So I guess it's really about whether it makes sense for Serverless to pass in the "cooked" values here so config script authors don't have to take extra steps to find them as well. In that case, this is probably just a few lines of code and no breaking changes to the release to just check the type of what serverless.js exports, and if it's a function, call it with those options as a parameter. Would you be open to a PR that implements this?

Something just this simple would do it:

let config = require(serviceFilePath);

if (_.isFunction(config)) {
    config = config(options);
} else if (!_.isPlainObject(config)) {
    throw new Error('serverless.js must export plain object');
}

@HyperBrain
Copy link
Member

@crrobinson14 It is just not easy as that - The command line options are not necessarily a raw input for the serverless configuration, because they are evaluated and validated first (even before the variable resolution takes place). All these things are dependent on the configuration, so the configuration cannot be dependent on them or it might create issues within the framework. The configuration itself is the root to build Serverless' context (including possibly transformed input parameters), so the context is only available after the configuration has been evaluated. The Serverless framework is built on this assumption. Changing things here would introduce a conceptual change.

Additionally, as soon as you allow exporting a function you would have to allow the function to return a promise, because then you also want to create the configuration upon results of asynchronous operations ... Imo this opens a big can of worms.

@HyperBrain
Copy link
Member

HyperBrain commented Feb 1, 2018

BTW: To just retrieve the arguments, you could use @pavelvlasov 's suggestion of utilizing process.argv

@crrobinson14
Copy link

@HyperBrain I hear what you're saying but what I'm advocating here is really a much smaller concept. For instance, I have zero problem saying that function can't be Promise-based. I don't think it's a given that this has to be so. If you felt it would open things up to developers "messing around" I would also have zero problem passing in a copy of the options (so the function can't mutate the object). But just what I've posted above would be INCREDIBLY useful for some projects I'm working on now. Would you be favorable to a PR that just did that one small thing?

@erikerikson
Copy link
Member

For natural and understandable reasons, this PR doesn't seem like it was fully baked.

I would have at least expected to be able to export an object or function which returned either an object or a promise that resolves to an object. Of course this opens a greater number of concerns, as @HyperBrain notes. That said, it's easy to const service = require(serviceFilePath); Promise.resolve(service) to deal with the resolution possibilities uniformly but that's the least of the concerns.

There is an inception problem here, as also noted by @HyperBrain, as a result of previous decisions to allow serverless configuration to be declared in the service definition. This includes fundamental configuration such as stage, region, and credentials. This capability offers increased repeatability and avoids pushing such details into adjoined scripts. This is highly valuable. Because of this, it also would be possible to supply a command line option, use it, and then override it via your returned configuration. Better yet, to supply credentials for use in executing serverless.js that would switch them out for (potentially part of, see #4702) variable resolution and command execution. mind-crush = 'on' for someone that unintentionally treads those waters, tries to help that person, or debug and fix that issue. This would be surprisingly easy to accomplish in an aged codebase that in large organizations would likely be maintained by separate engineers.

At least then using this serverless.js capability implies increased cognitive load and a restricted available context. Perhaps validation of consistency across the contexts could make this better (e.g. that region isn't changed by the returned configuration [but would some use case elicit an objection to the enforcement of this policy? Defaults/overrides/overwrites present much opportunity.]). That immediately increases the difficulty and tribal knowledge load of making any change related to any core framework setting (and which are those?).

I kinda couldn't care less about eslint flagging metacode. You can escape the$ in a template string just fine. I've had to do this writing tests for the variable system. It's ugly but sometimes those're the bullets you take and there's always eslint-disable-line.

I'd be happy enough to pass a half baked Config to an exported function, if not more. Making that config immutable is one reasonablish solution for avoiding a few problems and thereby a good suggestion @crrobinson14. It would, of course, be preliminary configuration and would require lengthy documentation. However someone is guaranteed to say "you can do this thing" on a forum or StackOverflow question, sans documentation, and then that'll come at us.

Stepping back, this is not a capability to stumble our way into. The highly probable result of that choice would the retraction of the feature and potentially a fracturing of the community. It may be already that the feature should be retracted. One can always and already write a script that generates a serverless.yml for subsequent consumption by the framework. That does, of course, contradict the principle of avoiding spread into adjoining resources (i.e. scripts).

Something that is very unclear is the relative priority of this. We don't have sane IAM role definition, programmatic invocation, and a slew of other features.

@crrobinson14
Copy link

Just before we get too far afield, there's a big difference between config and options. What I'm looking for here is a clean way to access options. That's all. It's absolutely true these can be accessed using something like commander or optimist but that seems like a bad approach to me for two reasons:

  1. Serverless has already done this evaluation, so it's duplicate work.
  2. Serverless itself could change how it processes options. Encouraging people now to directly access these themselves could lead to future complaints if this mechanism is changed. It seems like a slippery slope to recommend this path. It adds an unnecessary burden on the core maintainers to factor in this new potential for breaking changes in future updates.

I for one heartily support this new approach. I've done exactly the "generate a serverless.yml" approach and it's messy and antiquated compared to how other build tools work these days. I just think addressing this small detail now makes a lot of sense - not too many people have started using the new feature yet (it's only a few days old), it could avoid unnecessary future pain for multiple groups of people, and it's just a few lines of code.

@erikerikson
Copy link
Member

Sorry @crrobinson14. As I load this into my brain (recent entrant but involved with the variables code base a bit historically and in it right now) I've started with a wide, cautionary viewpoint.

At this point, removing this feature would be a breaking change and while I don't have the power to guarantee it remains, there also appears to be universal agreement among maintainers as to the utility of the feature.

Your points regarding the avoidance of duplicated option processing are well taken.

The export of an object is simplistic with few additional commitments over the current state. The introduction of options or any other enrichment of the API here introduces a much greater and more subtle set of guarantees, codification of patterns that have been largely "as it is" (receiving, thusly, less scrutiny), and semantic obligations.

FWIW, I'm trying to advocate this be done properly so as to increase the probability of long term success/sustainability of the feature.

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.

None yet

5 participants