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 aws-kotlin-jvm-maven template #4220

Merged
merged 4 commits into from
Sep 8, 2017
Merged

Add aws-kotlin-jvm-maven template #4220

merged 4 commits into from
Sep 8, 2017

Conversation

RafalWilinski
Copy link
Contributor

@RafalWilinski RafalWilinski commented Sep 6, 2017

What did you implement:

Closes #3883

Adds Kotlin template. It uses JVM and Maven under the hood. Support for gradle based is coming soon.

I'm also willing to investigate Kotlin -> JS compilation so it might run using node6.10 runtime.

How can we verify it:

serverless create -t aws-kotlin-jvm-maven
serverless deploy

It's also compatible with #4199 so you can use invoke local too.

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

Copy link
Contributor

@pmuens pmuens left a comment

Choose a reason for hiding this comment

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

Great addition @RafalWilinski 👍

Just added a comment about the template naming. Haven't tested it yet, but will do so in the upcoming days.

There are still some other things which needs to be added / updated. Best way to find them is to run a global search for aws-python3.

Let us know if you need any help here 💪

@@ -16,6 +16,7 @@ const validTemplates = [
'aws-groovy-gradle',
'aws-java-maven',
'aws-java-gradle',
'aws-kotlin-jvm-maven',
Copy link
Contributor

Choose a reason for hiding this comment

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

The current naming convention is provider-language/runtime-buildenv.

Maybe we should rename the template to aws-kotlin-maven (since AFAIK AWS uses the jvm behind the scenes)?

Copy link
Contributor Author

@RafalWilinski RafalWilinski Sep 7, 2017

Choose a reason for hiding this comment

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

I've added JVM part because Kotlin can compile to Java (JVM) or JS (Node.js). In future, I wanted to add also Kotlin -> JS, hence the name. Node.js option might be even more viable because of much faster initial start time vs JVM.

@RafalWilinski
Copy link
Contributor Author

RafalWilinski commented Sep 7, 2017

Thanks for review. I totally overlooked other things, sorry for that! Fix incoming

@eahefnawy
Copy link
Member

@RafalWilinski thanks a lot for that! Does this template work with your Invoke Local Java PR?

Looks good 👍 Nothing to add 😊

@RafalWilinski
Copy link
Contributor Author

Yes, It works with invoke local for Java.

Copy link
Contributor

@pmuens pmuens 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 updating the PR @RafalWilinski 👍

Just tested this template today and it's working great 💪! Good job 💯 (as usual!) --> Merging :shipit:

@pmuens
Copy link
Contributor

pmuens commented Sep 8, 2017

I'm also willing to investigate Kotlin -> JS compilation so it might run using node6.10 runtime.

BTW @RafalWilinski that would be super nice ☝️ 💯

@pmuens pmuens added this to the 1.22 milestone Sep 8, 2017
@pmuens pmuens merged commit 3b92e2f into serverless:master Sep 8, 2017
@RafalWilinski RafalWilinski deleted the aws-kotlin-template branch September 8, 2017 12:19
@eahefnawy eahefnawy mentioned this pull request Sep 13, 2017
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.

3 participants