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

Skip service_enable if already enabled #886

Merged
merged 1 commit into from
Apr 19, 2024

Conversation

brianphaley
Copy link

Repeatedly calling service_enable for a service that is already enabled can lead to unintended consequences. Some charms frequently call servie_resume which will call service('enable') and this adds a check to only do so of the service is not enabled.

Related-Bug: #2058505
(cherry picked from commit a334500)

Copy link
Author

@brianphaley brianphaley left a comment

Choose a reason for hiding this comment

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

From looking at the failure looks like we need this change:

0f7ca53

I'll cherry-pick that in another pull request.

@fnordahl
Copy link
Contributor

fnordahl commented Apr 5, 2024

Thank you for putting forward multiple options to solve this, I chose to merge #890 which would allow us to consider this PR in a uniform manner along with the other backports. Would you be able to rebase?

@brianphaley
Copy link
Author

Sure, I can rebase this one. I threw the squashed option because i wanted them both tested together, but github had other plans. The cherry-pick for 890 at least fixed the unit test failure locally.

Repeatedly calling service_enable for a service that
is already enabled can lead to unintended consequences.
Some charms frequently call servie_resume which will
call service('enable') and this adds a check to only
do so of the service is not enabled.

Related-Bug: #2058505
(cherry picked from commit a334500)
Copy link
Contributor

@fnordahl fnordahl left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@dosaboy dosaboy merged commit ee21759 into juju:stable/zed Apr 19, 2024
4 checks passed
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.

3 participants