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

make ruby tests pass #53

Open
wants to merge 4 commits into
base: PRODUCT-959/fun-add-ruby
Choose a base branch
from

Conversation

jameselkins
Copy link

@jameselkins jameselkins commented May 2, 2020

  • change handler to conform to test expectation
  • change bootstrap.rb server address to use AWS_LAMBDA_RUNTIME_API from ENV
  • change bootstrap.rb to load lambda handler properly from LAMBDA_TASK_ROOT
  • add ruby2.5 and ruby2.7 runtimes
  • add install-ruby script
  • use rvm to install rubies
  • add ruby-version test for ruby2.7 and ruby2.7
  • adjust ruby-hello test to use JSON generation

- change handler to conform to test expectation
- change bootstrap.rb server address to use AWS_LAMBDA_RUNTIME_API from ENV
- add ruby2.5 and ruby2.7 runtimes
- add install-ruby script
- use rvm to install rubies
- add ruby-version test for ruby2.7 and ruby2.7
@jameselkins
Copy link
Author

hey @styfle - what's the best way to ask for a review / discuss this with you? I'm a little new to contributing so however you'd prefer I format my pull request, I'd be happy to do so.

Additionally, I noticed that this is probably not going to work correctly end-to-end unless:

  1. https://github.com/zeit/now/blob/master/packages/now-cli/src/util/dev/builder.ts#L362 is cast as a RuntimeLiteral (or something else needs to happen? Right now it complains about that when I try to build, I've changed that line to Runtime: asset.runtime as RuntimeLiteral locally)
  2. https://github.com/zeit/now/blob/master/packages/now-ruby/install-ruby.ts#L53-L61 is modified to look for the correct gemHome if meta.isDev === true
  3. https://github.com/zeit/now/blob/master/packages/now-ruby/install-ruby.ts#L21-L24 needs to be removed of course :)

Please let me know your thoughts on the code!

@jameselkins
Copy link
Author

Ah, I see now about vercel/vercel#3667 - Would make sense to modify that PR a bit more to take my suggestions above into account?

src/runtimes.ts Outdated Show resolved Hide resolved
src/install-ruby.ts Outdated Show resolved Hide resolved
src/install-ruby.ts Outdated Show resolved Hide resolved
Copy link
Member

@styfle styfle 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 left a few comments.

- remove any mention of RuntimeLiteral type
- change install-ruby.ts script to install rvm to a temp directory
- adjust ruby-hello test to use JSON generation
@jameselkins
Copy link
Author

hey @styfle - thanks for the suggestions! I made the updates you recommended :)

@@ -1,30 +1,8 @@
export type RuntimeLiteral =
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we want to remove this though 🤔

@wyattades wyattades mentioned this pull request Apr 9, 2021
4 tasks
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.

2 participants