-
Notifications
You must be signed in to change notification settings - Fork 2
Put the Evaluator into a Docker container to isolate important resources from any third-party-run code #38
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
Conversation
…eroku login credentials
Looks good! However, I would suggest to use https://github.com/marcuslonnberg/sbt-docker to have everything in the sbt ecosystem ;) Here, one example that I used some time ago: https://github.com/47deg/spark-on-lets-code/blob/master/project/SettingsDocker.scala |
@juanpedromoreno That looks awesome, thanks. I'll do that as it looks like it will remove most of the code I put into the |
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.
LGTM, We should have some test in this PR or a different one to ensure some of the security issues we were trying to alleviate with this PR are addressed. Think about calls to sys.env, sys.exit or whatever relevant this fixes.
Note, still need to use the `docker` service in Travis, as you cannot programmatically log in to private repositories, like Heroku. See marcus-drake/sbt-docker#28 for more details.
We don't need to do the assembly step until the docker image is created
@juanpedromoreno would you be able to review my changes for using sbt-docker please? As disussed with @raulraja, perhaps we should move forward with getting this live before I finish writing the end-to-end tests to alleviate the mentioned above. @raulraja as for the next stages, I'd take your lead here. Currently this is configured against http://scala-evaluator-sandbox.herokuapp.com/eval - which is configured with a "hobby" dyno rather than the professional one - and no other Heroku configuration. Our options:
I'd suggest the first option as that gives us more control should things go wrong. And then once we're fully migrated, we can then rename this app to scala-exercises and drop the sandbox one. Your call. |
Sounds good, let's keep it in the sandbox one for now so we can also test Kazari against it before migrating Scala Exercises. |
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.
LGTM!
This PR puts the Evaluator in a Docker container to isolate information such as Heroku API keys and passwords from any third party code, such as looking at
System.getEnv
.While this PR is relatively small, it has changed the way the Evaluator is built and deployed. This is currently being built and deployed to a separate Heroku application, called
scala-evaluator-sandbox
As such, until an appropriate time after acceptance on this PR, when everyone is happy, we can replace the currentscala-evaluator
application deployment with this.The Travis CI now builds a full "fat" jar and performs the deployment of that jar to Heroku inside a Docker container when committing to the
master
branch (that is, on successful PR merge).This is my first foray into the exciting world of Docker, so if anyone with more experience (or not) spots anything I'm doing fundamentally wrong, please let me know.
Right now, this application is deployed at the http://scala-evaluator-sandbox.herokuapp.com/eval endpoint, so feel free to poke it there to see how it works - it is behind a dyno, so it recovers from
System.exit
. There should be no difference in the running code between this and the one atscala-evaluator.herokuapp.com
.@raulraja @juanpedromoreno @dialelo Would you be able to take a look please? Is there anyone else who may be appropriate to take a look?
Resolves #6.
Incidentally, it's really nice that Heroku can run pretty much anything now.
Thanks!