[Fix #2418] Allow function event definitions to be variables

[fruffin]

What did you implement:

_Implementing Issue:_ #2418

We used to be able to specify a "file variable" for function event configuration, which was quite useful for a service with multiple handlers. This allowed each handler to be in their own directory, along with the event configuration. This looked something like this:

functions:
  users:
    handler: handlers/users/handler.users
    events: ${file(./handlers/users/config.yml):events}

The config.yml file would contain a valid event configuration.

Recently, an additional check was added to make sure event configurations are arrays, and it is no longer possible to use variables for event configurations and instead the following error is thrown:

Events for "users" must be an array, not an string

This pull request addresses this issue to restore functionality

How did you implement it:

I have moved the service validation code from the load function to a new validate function in the service class, which now gets called from the serverless class once the variables have been populated.
This can later be extended to support more thorough validation of the service.

How can we verify it:

Given a new serverless service with 2 handlers: posts and users, the serverless.yml file could look like this:

functions:
  posts:
    handler: handlers/posts/handler.posts
    events: ${file(./handlers/posts/config.yml):events}
  users:
    handler: handlers/users/handler.users
    events: ${file(./handlers/users/config.yml):events}

And the posts config.yml file could look like this:

events:
  - http:
      method: GET
      path: posts/{id}
      request: ${file(./templates.yml):request}
      cors: ${self:custom.currentVars.cors}
      integration: lambda
  - http:
      method: POST
      path: posts
      request: ${file(./templates.yml):request}
      cors: ${self:custom.currentVars.cors}
      integration: lambda

I have added unit tests to verify this and have also tested it successfully on one of my projects

Todos:

• Write tests
• Write documentation
• Fix linting errors
• Make sure code coverage hasn't dropped
• Provide verification config/commands/resources
• Change ready for review message below

_Is this ready for review?:_ YES

[fruffin]

Ready for review

[flomotlik]

Thanks for submitting this. Looked over the PR and I think we should implement this differently. Basically validation that the config is fine should happen after variables get populated. Otherwise we'll keep adding special cases for things like this.

@eahefnawy @pmuens how about we change the service to have a load and a validate command, so loading really only loads variables, then we run through the populate variables command in the Serverless class and afterwards we call validation on the service?

[fruffin]

That's a great idea.
Let me know what you guys decide, I wouldn't mind giving it a crack

[fruffin]

I gave it a go this morning. The code change was relatively simple and it all seems to work well.
However, I'm struggling with the tests. For some reason I can't get serverless.variables.populateService() to "see" any of the custom variables I have added...

[fruffin]

ok, I found what the problem was.
Good to go!

2 things to note however:

• There are some validation elements which I have left in the load function as they seem critical to the rest of the function execution
• There is a lot more that can be validated in the validate function, like for example that the structure of the events is valid, and not just checking that it is an array. This can be completed over time.

What do you think?

[fruffin]

@flomotlik What's the review process for pull requests? I found the pull request pipeline for this project, but for some reason this pull request seems to be the only one missing from the list. Have I missed anything?

[fruffin]

@flomotlik @eahefnawy @pmuens any chance to get this one looked at soon so I don't have to resolve conflicts every few days?

Thanks!

[nfour]

Also keen on this getting merged in.

Having to create a serverless.template.yml and building apon that to generate serverless.yml till this is in. Pretty gross.

[securityvoid]

I ran into this while looking into an issue I submitted (Issue #2997 ).

In its current form this will still error out if you attempt to use ${file(filename)} at the functions: level.

Looking through most of the "load" function, a lot of it is setting default values or validating values. If the goal is truly to have everything evaluated after all of the variables are resolved, I almost wonder if mostly everything in the load function of Service.js should move over to the validate.

If I have some time later I'll play around with that and see if that still works.

[fruffin]

@flomotlik @eahefnawy @pmuens where are we at with this? It's been over 2 months since the original PR... Anything you need me to change?
Thanks

[igorshmukler]

tried this, and it solved the problem for me. thank you @fruffin
would love to see this merged!

[fruffin]

@flomotlik @eahefnawy @pmuens I would really love to get some feedback on this so it can get merged/fixed. I've been using this version for my projects for the last 3 months, but it's a bit annoying to have to merge the latest "released" changes to this temporary version all the time so my projects can keep going forward.
Any ETA on this one?

Thanks!

[eahefnawy]

@fruffin really sorry this slipped out of my attention 😞 ... imo supporting your use case as laid in the issue is more important than validating the events is an Array type. Even better is how you handled it.

It's working great on my machine too, so this is G2M 😊 ... Will be included in the next release 🙌

[fruffin]

Thanks @eahefnawy !

[nfurfaro]

I'm curious... With validation happening after variable population, will this mean that I can now define my actual function in an external functionFoo.yml file and reference it with a ${file(functionFoo. yml)} in the function section of my serverless. yml, similar to how we can reference external resources? Ive been trying to make this work for a while now and it seemed like the order of validation - > population needed to be swapped for it to work properly.

[pmuens]

@nfurfaro interesting idea!

Do you mean that the order of validation / populations should be swapped after this PR was merged?

[fruffin]

@pmuens this PR is essentially swapping this already. The validation was happening too early causing the external file reference to be validated before it was "expanded". What Nick is referring to is one level higher than the issue I initially raised, but I think the issue is the same
@nfurfaro In theory your scenario should work with this PR, but I haven't tried it yet

[pmuens]

@fruffin thanks for the clarification! 👍

[pgasiorowski]

Big thanks for adding this switch. Finally can update to the latests.