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 Ruby to Supported Runtime Enum #866

Merged
merged 9 commits into from
Dec 21, 2018
Merged

Conversation

awood45
Copy link
Member

@awood45 awood45 commented Dec 18, 2018

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.

@awood45 awood45 mentioned this pull request Dec 18, 2018
6 tasks
@awood45
Copy link
Member Author

awood45 commented Dec 18, 2018

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"):
Copy link
Contributor

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 \
Copy link
Contributor

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.

Copy link
Contributor

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

@sriram-mv
Copy link
Contributor

This looks good, @awood45 do you want to add an integration test for sam build with ruby as well? and we can merge this.

@sriram-mv
Copy link
Contributor

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
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Contributor

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:

  1. Do we need to tell Ruby developers how they should install Ruby?
  2. Customers trying Ruby quickstart should have Ruby installed, shouldn't we assume that and cut the Installation part?

Copy link
Contributor

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
Copy link
Contributor

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

Copy link
Member Author

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?

  1. 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.
  2. I may not want all of my dependencies in every function.
  3. 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.

Copy link
Member Author

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
Copy link
Contributor

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)

Copy link
Contributor

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.

awood45 and others added 7 commits December 20, 2018 14:06
WIP: Unit tests may have a notion of a central Gemfile that isn't
compatible with per-function Gemfile pattern that `sam build` expects.
Global Gemfile for tests. Gemfile in the function for use in tests.

Hat tip to lauratpa and her PR aws#860 which is the core of this part of
the change set.
@sriram-mv sriram-mv merged commit 8f4b180 into aws:develop Dec 21, 2018
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