Skip to content

Conversation

@sami-cseseu
Copy link
Contributor

Added stop, version functionality for Delphi management

@sami-cseseu sami-cseseu requested a review from bhermann October 2, 2018 09:44
@codecov
Copy link

codecov bot commented Oct 2, 2018

Codecov Report

Merging #21 into develop will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@          Coverage Diff          @@
##           develop   #21   +/-   ##
=====================================
  Coverage        0%    0%           
=====================================
  Files            8     9    +1     
  Lines          172   184   +12     
  Branches        15    18    +3     
=====================================
- Misses         172   184   +12
Impacted Files Coverage Δ
app/EagerLoaderModule.scala 0% <ø> (ø) ⬆️
app/services/StartUpService.scala 0% <ø> (ø) ⬆️
app/utils/AppLogging.scala 0% <ø> (ø) ⬆️
...pp/utils/instancemanagement/InstanceRegistry.scala 0% <ø> (ø) ⬆️
app/utils/Configuration.scala 0% <ø> (ø) ⬆️
app/utils/instancemanagement/Instance.scala 0% <ø> (ø) ⬆️
app/utils/BlockingHttpClient.scala 0% <ø> (ø) ⬆️
app/controllers/SettingsController.scala 0% <0%> (ø)
app/controllers/HomeController.scala 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b1f339...3a3a920. Read the comment docs.

@ghost ghost assigned sami-cseseu Oct 2, 2018
@ghost ghost added the review label Oct 2, 2018
Copy link
Member

@bhermann bhermann left a comment

Choose a reason for hiding this comment

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

This PR introduces an endpoint that allows anyone to stop the web application.
This is critical and allows a very easy denial-of-service attack.
This functionality should be protected with an API key at least.
Also, while sys.exit(1) is effective, it has two problems: 1) return code 1 conveys that the exit was because of a fault, 2) it does not gracefully shut down the actor system, thus, terminating open connections, etc.
Please adapt the current solution.

@bhermann
Copy link
Member

Please clarify the relation of the PR and PR #22. I am confused which one is current as both make quite similar changes.

@johannesduesing
Copy link
Contributor

As far as i can see, this PR is fully contained in #22 and should therefore be closed. Since the management functionality added in this PR is actually needed for the implementation of the new Instance Registry API (see #22) it does not make sense to have a dedicated PR for this in my opinion. @sami-cseseu do you agree ? If so, please close this PR.

@ghost ghost removed the review label Nov 4, 2018
@johannesduesing johannesduesing deleted the feature/mangementFunctionality branch November 4, 2018 17:46
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.

4 participants