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 ControlServices comply with autostart server settings #705

Merged
merged 1 commit into from
Dec 16, 2024

Conversation

Enet4
Copy link
Collaborator

@Enet4 Enet4 commented Nov 13, 2024

  • do not start the service(s) if their respective autostart setting is set to false, a regression probably existing since Dicoogle 3.0.0
  • remove unused variable webServicesRunning in ControlServices
  • remove use of raw types in startStorage()
  • remove outdated TODO note
  • refactor ControlServices#startStorage() to return whether the service started (true) or was already running (false) and raise an exception on error
  • adjust ServicesServlet according to changes to ControlServices, and for the output to be more precise and informative

- do not start the service(s) if their respective autostart setting is set to false,
  a regression probably existing since Dicoogle 3.0.0
- remove unused variable `webServicesRunning` in ControlServices
- remove use of raw types in startStorage()
- remove outdated TODO note
- refactor ControlServices#startStorage() to return and raise exception
- adjust ServicesServlet according to changes to ControlServices
  and to be more informative
Copy link
Collaborator

@tiberio-baptista tiberio-baptista left a comment

Choose a reason for hiding this comment

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

Autostart is now being respected, and I've confirmed this with a local run.

I've confirmed that ControlServices#startStorage() behaves as suggested in the PR.

I did find an issue with ServicesServlet: though I'm not sure that this was present before, I noticed that GET /management/plugins/ returns the following response body:

{
  "isRunning": false,
  "port": -1,
  "autostart": false
}

Please confirm if this is expected. Otherwise, the PR is good and I can approve.

Personally, I would expect this endpoint to list the available plugins.

@Enet4
Copy link
Collaborator Author

Enet4 commented Dec 12, 2024

I did find an issue with ServicesServlet: though I'm not sure that this was present before, I noticed that GET /management/plugins/ returns the following response body:

{
  "isRunning": false,
  "port": -1,
  "autostart": false
}

Please confirm if this is expected. Otherwise, the PR is good and I can approve.

Personally, I would expect this endpoint to list the available plugins.

That is a curious finding. That endpoint is not part of the API, but it was probably captured by the service status servlet anyway, which tried to fetch the status of a service which does not exist.

This can be improved, but it is hardly problematic, nor does it not look like a regression from this PR.

@tiberio-baptista
Copy link
Collaborator

This can be improved, but it is hardly problematic, nor does it not look like a regression from this PR.

For clarity's sake, I only found and tested that endpoint because I went into the code to see what endpoints that servlet was mapped to, and that one was included.

I have created a new issue to track this, though of course it is of low priority. With nothing else to add, I'll be approving this PR.

@tiberio-baptista tiberio-baptista self-requested a review December 16, 2024 09:24
Copy link
Collaborator

@tiberio-baptista tiberio-baptista left a comment

Choose a reason for hiding this comment

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

Since the servlet plugin endpoint is not currently intended for use, I'll be approving this PR. Looks good!

@Enet4 Enet4 merged commit 9285b39 into dev Dec 16, 2024
2 checks passed
@Enet4 Enet4 deleted the bug/service-autostart branch December 16, 2024 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants