-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 Ruby to Supported Runtime Enum #866
Conversation
I'll say that, right now, the example function is breaking when packaged and deployed. I'm investigating why this is. |
@@ -57,6 +57,12 @@ def _get_workflow_config(runtime): | |||
dependency_manager="npm", | |||
application_framework=None, | |||
manifest_name="package.json") | |||
elif runtime.startswith("ruby"): |
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.
Woot! sam build for ruby! :)
|
||
```bash | ||
sam package \ | ||
--template-file template.yaml \ | ||
--template-file .aws-sam/build/template.yaml \ |
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.
Need to investigate this, thought there was some code that automatically picked this up.
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.
No need to add --template-file
when using build anymore. We will auto pick up build if it exists otherwise default to template.yaml
.
https://github.com/awslabs/aws-sam-cli/blob/develop/samcli/commands/package/__init__.py#L21
and
https://github.com/awslabs/aws-sam-cli/blob/develop/samcli/commands/_utils/options.py#L35
This looks good, @awood45 do you want to add an integration test for sam build with ruby as well? and we can merge this. |
The integ tests worked locally, need to get a new release of the builders out to get the new docker images. |
rvm install ruby-2.5.0 | ||
rvm use ruby-2.5.0 | ||
rvm --default use 2.5.0 | ||
rvm install ruby-2.5.3 |
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.
Does patch version need to be specified here?
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.
It's a best practice, yes. Ruby on AWS Lambda uses version 2.5.3 - while 2.5.0 will technically work for builds, there are security patches present on 2.5.3 that are not present on 2.5.0, so I would not intentionally direct users to use an older version in this case.
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.
Is there a way the newest just gets installed. I am worried this will be out of date and something we need to maintain.
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.
do we need to provide instructions on how to use install Ruby for Ruby developers? I wonder if Requirements (ruby 2.5) suffices as we do with Python and Node init
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 how you install Ruby. Not sure what the question is @heitorlessa
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.
Sure, let me try once more and let me know if it's clearer:
- Do we need to tell Ruby developers how they should install Ruby?
- Customers trying Ruby quickstart should have Ruby installed, shouldn't we assume that and cut the Installation part?
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.
I wouldn't add install instructions. People who use languages like ruby, go etc will have their tooling in place.
It would be like every pkg internally saying start off by installing the Amazon builder tools.
@@ -7,7 +7,8 @@ This is a sample template for {{ cookiecutter.project_name }} - Below is a brief | |||
├── README.md <-- This instructions file | |||
├── hello_world <-- Source code for a lambda function | |||
│ ├── app.rb <-- Lambda function code | |||
├── Gemfile <-- Ruby dependencies | |||
│ ├── Gemfile <-- Ruby function dependencies | |||
├── Gemfile <-- Ruby test/documentation dependencies |
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.
Not a huge fan of the Double Gemfile
. How do I know or remember which one to update? Is this absolutely needed? For Node, we had tests under the function directory and excluded them through .npmignore
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.
So...I'm open to ways we can support this through sam build
. If we had to pick one, we would not include the global test/docs Gemfile, but then the way the example code does testing wouldn't work easily. I don't know how to get SAM CLI to use a Gemfile in the root directory to build vendored dependencies into each function directory in a single command.
It's actually very possible that multiple Gemfiles may be a reality, and that we just need to teach the paradigm. Why?
- Layers will eventually be a part of this build story, and you only want to specify those dependencies within the layer itself. In fact, we may want builds where we say "don't fetch all dependencies I need, expect some to be resolved in a layer" down the road.
- I may not want all of my dependencies in every function.
- I may want global dependencies in my build structure.
There is actually precedent for this, the Ruby SDK repo itself: https://github.com/aws/aws-sdk-ruby/
We have a global Gemfile for testing, building, and documentation generation. Within each modular package, we have its own gemspec for only the required dependencies. This may be the correct pattern for modular Lambda functions.
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.
I can image an appropriate alternative approach, perhaps where we use "groups" per function to keep dependency versions in sync in a global Gemfile. In an ideal world, we could provide options to the user.
@@ -630,7 +630,7 @@ def make_service(list_of_routes, function_provider, cwd): | |||
|
|||
def make_service_response(port, method, scheme, resourcePath, resolvedResourcePath, pathParameters=None, | |||
body=None, headers=None, queryParams=None, isBase64Encoded=False): | |||
response_str = '{"httpMethod": "GET", "body": null, "resource": "/something/{event}", "requestContext": {"resourceId": "123456", "apiId": "1234567890", "resourcePath": "/something/{event}", "httpMethod": "GET", "requestId": "c6af9ac6-7b61-11e6-9a41-93e8deadbeef", "accountId": "123456789012", "stage": "prod", "identity": {"apiKey": null, "userArn": null, "cognitoAuthenticationType": null, "caller": null, "userAgent": "Custom User Agent String", "user": null, "cognitoIdentityPoolId": null, "cognitoAuthenticationProvider": null, "sourceIp": "127.0.0.1", "accountId": null}, "extendedRequestId": null, "path": "/something/{event}"}, "queryStringParameters": null, "headers": {"Host": "0.0.0.0:33651", "User-Agent": "python-requests/2.20.0", "Accept-Encoding": "gzip, deflate", "Accept": "*/*", "Connection": "keep-alive"}, "pathParameters": {"event": "event1"}, "stageVariables": null, "path": "/something/event1", "isBase64Encoded": false}' # NOQA | |||
response_str = '{"httpMethod": "GET", "body": null, "resource": "/something/{event}", "requestContext": {"resourceId": "123456", "apiId": "1234567890", "resourcePath": "/something/{event}", "httpMethod": "GET", "requestId": "c6af9ac6-7b61-11e6-9a41-93e8deadbeef", "accountId": "123456789012", "stage": "prod", "identity": {"apiKey": null, "userArn": null, "cognitoAuthenticationType": null, "caller": null, "userAgent": "Custom User Agent String", "user": null, "cognitoIdentityPoolId": null, "cognitoAuthenticationProvider": null, "sourceIp": "127.0.0.1", "accountId": null}, "extendedRequestId": null, "path": "/something/{event}"}, "queryStringParameters": null, "headers": {"Host": "0.0.0.0:33651", "User-Agent": "python-requests/2.20.1", "Accept-Encoding": "gzip, deflate", "Accept": "*/*", "Connection": "keep-alive"}, "pathParameters": {"event": "event1"}, "stageVariables": null, "path": "/something/event1", "isBase64Encoded": false}' # NOQA |
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.
grrr.. let's just pin requests. We have installers now, so pinning isn't as dangerous. If customers run into conflicts, this gives them a reason to get onto our installers and out of pip (which is what we want/recommend anyways)
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.
We could remove requests from test requirements (dev.txt) , and pin the requests in the base.txt.
But if we pin, we would periodically need to bump up requests as when there is a release of requests.
WIP: Unit tests may have a notion of a central Gemfile that isn't compatible with per-function Gemfile pattern that `sam build` expects.
402f234
to
381a7c5
Compare
This is required for the Ruby support added to aws-lambda-builders to work in SAM CLI.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.