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

Improve service restart behavior configuration #88

Conversation

hwo-wd
Copy link
Collaborator

@hwo-wd hwo-wd commented May 30, 2023

Add an extra parameter keycloak_service_restart_always which sets the systemd unit for keycloak to automaticcaly restart on the always condition. When keycloak_service_restart_always is True, keycloak_service_restart_on_failure must also be True.

Variable Description Default
keycloak_service_restart_always systemd restart always behavior activation False
keycloak_service_restart_on_failure systemd restart on-failure behavior activation False

@hwo-wd
Copy link
Collaborator Author

hwo-wd commented May 30, 2023

@guidograzioli, sry for the breaking change, but I guess it wasn't that widespread by now;
I didn't focus on downwards compatibility, let me know if you'd prefer a compatibility shim variable

@guidograzioli
Copy link
Member

Hello, thanks for the PR. I believe allowing all the systemd values is too generic and does not directly apply to the keycloak startup script. In particular, I don't find an use case of always, on-success and on-watchdog, while failure, abnormal and abort would fall down to the same case. Do you have a specific case that would require this configurability?

@hwo-wd
Copy link
Collaborator Author

hwo-wd commented May 31, 2023

I agree with on-success, on-watchdog etc. being superfluous.
Hear me out though: my motivation was to introduce always as some administrators prefer to have this in production over on-failure and while I was at it, I decided to make it more generic than my previous try ;-).

I could remove all except for no, on-failure, and always, would that be ok for you?

@guidograzioli
Copy link
Member

guidograzioli commented May 31, 2023

Ok, I see; how about this:

  • keycloak_service_restart_always: new parameter, default False. Used to set the systemd restart value always
  • keycloak_service_restart_on_failure remains as it is, default False, unless keycloak_service_restart_always is true, in which case by default is True. Sets the value as on-failure when true
    ?

And at the same time, the four parameters keycloak_service_* for tuning the restart behaviour, can you remove the jinja conditionals and just set to a static value? We can cope with having the variables defined even when the flags are False

@hwo-wd hwo-wd force-pushed the feature/improve_service_restart_behavior branch from aa57701 to 820cacb Compare May 31, 2023 16:11
@hwo-wd hwo-wd force-pushed the feature/improve_service_restart_behavior branch from 820cacb to bc4cb5c Compare May 31, 2023 18:29
@hwo-wd
Copy link
Collaborator Author

hwo-wd commented May 31, 2023

This should do the trick

Copy link
Member

@guidograzioli guidograzioli left a comment

Choose a reason for hiding this comment

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

LGTM

@guidograzioli guidograzioli merged commit db480d0 into ansible-middleware:main Jun 1, 2023
1 check passed
@guidograzioli guidograzioli added the minor_changes New parameters added to modules, or non-breaking behavior changes to existing parameters; no bugfix label Jun 1, 2023
@hwo-wd hwo-wd deleted the feature/improve_service_restart_behavior branch June 1, 2023 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor_changes New parameters added to modules, or non-breaking behavior changes to existing parameters; no bugfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants