Adding Java runtime. #94
Conversation
Codecov Report
@@ Coverage Diff @@
## master #94 +/- ##
==========================================
+ Coverage 83.9% 84.19% +0.28%
==========================================
Files 40 41 +1
Lines 1373 1398 +25
Branches 276 279 +3
==========================================
+ Hits 1152 1177 +25
Misses 221 221
Continue to review full report at Codecov.
|
|
@allen-servedio Fast work on getting this implemented! I'll code review tomorrow and let you know if I find anything. Can you add docs to the README for Java support? Follow the style here: Once this is ready, we should drop a PR to the main framework project. This will include adding docs which get imported on hosted on their site, a template project for the Java runtime and examples in the Thanks again for doing this! Congrats on the first PR to this project. Since you know your way around the code-base now, feel free to open issues and send PRs for other stuff you find! |
|
Okay @jthomas, pushed the docs (sorry I missed them initially). Let me know what changes you want and generally what you want in the PR for the examples, etc... |
README.md
Outdated
| runtime: java | ||
| functions: | ||
| hello: | ||
| handler: target/hello-world-1.0.jar |
There was a problem hiding this comment.
I'd prefer to provide both the JAR file and the main class in the handler parameter. This keeps it consistent with the other runtimes which use a file.method style. For this runtime, we would adopt the following:
handler: target/hello-world-1.0.HelloWorld
This should be a small change to your PR - I think if you back out the convertHandlerToPath & calculateFunctionMain fns it should just work?
There was a problem hiding this comment.
That is going to get a little gnarly if they follow the standard practice of NOT using the default package and actually place their class into a package. For example, if they put the class in a package called com.company.app then the handler will look like this:
handler: target/hello-world-1.0.com.company.app.HelloWorld
That is visually a bit hard to parse (and I THINK breaks the code you were referring to). I suppose you could do something like this:
handler: target/hello-world-1.0.jar:com.company.app.HelloWorld
The reason I used annotation in this pull request was I saw that was how you marked an action as a web action... So I was thinking annotations could be used to provide further refinements of how actions needed to be deployed. That being said, I understand what you mean by consistency, so definitely open to changing that part.
There was a problem hiding this comment.
I can see that might get a bit long. handler: target/hello-world-1.0.jar:com.company.app.HelloWorld is definitely a bit better style.
Annotations are actually passed through to the platform when creating the actions. They aren't used for meta-data inside the framework if you see what I mean?
I agree it does get a bit long but think the second style (with :) is the best compromise between consistency and readability.
|
This PR is looking really good - great doc updates! Everything looks fine apart from a small change to how we define the Thanks for the work on this 👍 |
…e to adding it after the jar with a colon between.
|
Okay changes pushed @jthomas . Let me know where I need to make the other doc changes and I can run up another PR. Thanks! |
|
This looks perfect, thanks for the changes. I'll make a note to get a release out with this in soon. |
3 participants
Closes #93
@jthomas - Okay, here is my attempt at adding the Java runtime. Apologies if there are some obvious NodeJS coding issues. Just let me know what you want altered.
I installed it locally and pushed up a simple Java action and was able to call it.