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
Added support for serverless.json. Improved error messages with filename #3647
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙏 This is really nice! Love it.
There are just a few tests missing. After that it should be GTM. Thank you very much @HyperBrain 👍 💯
} | ||
// List of supported service filename variants. | ||
// The order defines the precedence. | ||
const serviceFilenames = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unit tests which check for all the possible file extensions should be added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're right. I will add them - also a precedence check would be beneficial.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks 👍 +1 for the precedence check!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating @HyperBrain 💪
I tested it and it turns out that there are other parts in the codebase which needed an update (otherwise Serverless won't generate a .serverless
directory since the service path could not be determined).
I've pushed the changes with the tests for those pieces and tested it thoroughly with the following serverless
files:
serverless.yml
service: service
provider:
name: aws
runtime: nodejs6.10
functions:
hello:
handler: handler.hello
serverless.yaml
service: service
provider:
name: aws
runtime: nodejs6.10
functions:
hello:
handler: handler.hello
serverless.json
{
"service": "service",
"provider": {
"name": "aws",
"runtime": "nodejs6.10"
},
"functions": {
"hello": {
"handler": "handler.hello"
}
}
}
Everything works as a charm 🌟
GTM from my side.
Waiting for @eahefnawy and @brianneisler feedback
Oh, I missed these. Thanks :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just gave this another spin today and added some documentation updates.
Everything works fine! Will merge once the build passes 🎉
What did you implement:
Closes #3566
How did you implement it:
Service.load() now tries to find one of the supported service filename variants (
serverless.yaml
,serverless.yml
and newserverless.json
, with this precedence). As soon as one is found, it will be loaded. Additionally the filename is remembered and printed in the error messages. Previouslysome error .... in serverless.yml
was printed regardless, if the file was named .yaml or .yml.JSON service file support is added with no tech debt, as the used YAML parser is spec compliant (a JSON is itself a valid YAML per definition) and will load the JSON out-of-the-box.
How can we verify it:
Convert any existing service.yml to JSON using some tool. Start your favorite serverless command.
It should behave and load exactly as before.
Additionally you can do some tests with multiple service files (.yml, .yaml, .json) to test the precedence - although I regard this as an edge case caused by sloppy development. ;-)
Todos:
Is this ready for review?: YES
Is it a breaking change?: NO