Skip to content

Leave the details of service management to the distro.#1014

Closed
citrus-it wants to merge 1 commit into
canonical:mainfrom
omniosorg:service
Closed

Leave the details of service management to the distro.#1014
citrus-it wants to merge 1 commit into
canonical:mainfrom
omniosorg:service

Conversation

@citrus-it
Copy link
Copy Markdown
Contributor

@citrus-it citrus-it commented Sep 10, 2021 •
edited
Loading

Proposed Commit Message

Leave the details of service management to the distro.

Various modules restart services and they all have logic to try and
detect if they are running on a system that needs 'systemctl' or
'service', and then have code to decide which order the arguments
need to be etc. On top of that, not all modules do this in the same way.

The duplication and different approaches are not ideal but this also
makes it hard to add support for a new distribution that does not use
either 'systemctl' or 'service'.

This change adds a new manage_service() method to the distro class
and updates several modules to use it.

Checklist:

  • My code follows the process laid out in the documentation
  • I have updated or added any unit tests accordingly
  • I have updated or added any documentation accordingly

@citrus-it citrus-it force-pushed the service branch 4 times, most recently from 518d8b8 to fbe74bf Compare September 13, 2021 08:39
@citrus-it citrus-it marked this pull request as draft September 13, 2021 10:06
@citrus-it citrus-it marked this pull request as ready for review September 13, 2021 12:12
@citrus-it citrus-it force-pushed the service branch 3 times, most recently from 86a4812 to db4d34f Compare September 13, 2021 12:39
@citrus-it
Leave the details of service management to the distro.
ae507b8 
Various modules restart services and they all have logic to try and
detect if they are running on a system that needs 'systemctl' or
'service', and then have code to decide which order the arguments
need to be etc. On top of that, not all modules do this in the same way.

The duplication and different approaches are not ideal but this also
makes it hard to add support for a new distribution that does not use
either 'systemctl' or 'service'.

This change adds a new manage_service() method to the distro class
and updates several modules to use it.
@TheRealFalcon
Copy link
Copy Markdown
Contributor

TheRealFalcon commented Sep 13, 2021

Hey @citrus-it , I noticed a lot of recent pushes to this branch. Is it ready for review yet, or should I hold off for a while longer? If it isn't ready for review, it'd be best to set the wip label on the PR.

@citrus-it
Copy link
Copy Markdown
Contributor Author

citrus-it commented Sep 13, 2021

Hey @citrus-it , I noticed a lot of recent pushes to this branch. Is it ready for review yet, or should I hold off for a while longer? If it isn't ready for review, it'd be best to set the wip label on the PR.

Hi, sorry about that, I had a little trouble getting the integration tests to all pass. CI is clean now so it's ready for review.

TheRealFalcon
TheRealFalcon suggested changes Sep 15, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

Thanks! This is a great refactor overall.

I left a few inline comments, but nothing major.

Comment thread cloudinit/config/cc_rsyslog.py
Comment thread cloudinit/distros/__init__.py
Comment thread tests/unittests/test_distros/test_manage_service.py
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Sep 30, 2021

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging mitechie, and he will ensure that someone takes a look soon.

(If the pull request is closed and you would like to continue working on it, please do tag mitechie to reopen it.)

@github-actions github-actions Bot added the stale-pr Pull request is stale; will be auto-closed soon label Sep 30, 2021
@citrus-it
Copy link
Copy Markdown
Contributor Author

citrus-it commented Oct 6, 2021

Sorry for the delay on this, I'll go through the comments and update the PR at the weekend.

@github-actions github-actions Bot closed this Oct 14, 2021
@TheRealFalcon
Copy link
Copy Markdown
Contributor

TheRealFalcon commented Oct 16, 2021

@citrus-it , are you still working on this? Would you like this re-opened?

@citrus-it
Copy link
Copy Markdown
Contributor Author

citrus-it commented Oct 20, 2021

@citrus-it , are you still working on this? Would you like this re-opened?

Yes please, sorry again for the delay, I'll push an updated commit today.

@TheRealFalcon
Copy link
Copy Markdown
Contributor

TheRealFalcon commented Oct 20, 2021

@citrus-it , Github won't let me reopen it. Feel free to create a new PR with a link to this one.

@citrus-it citrus-it mentioned this pull request Oct 20, 2021
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@TheRealFalcon TheRealFalcon TheRealFalcon requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

stale-pr Pull request is stale; will be auto-closed soon

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@citrus-it @TheRealFalcon