Skip to content

Conversation

carbonin
Copy link
Member

As init has been deprecated it is quickly becoming necessary
to use systemctl in its place.

As init has been depricated it is quickly becoming necessary
to use systemctl in its place.
@chessbyte
Copy link
Member

@Fryguy please review

@Fryguy
Copy link
Member

Fryguy commented Jul 24, 2015

I'd prefer to name the module generically (that way one could write an init subclass and a systemctl subclass and a "whatever-some-other-OS" uses subclass.

@carbonin
Copy link
Member Author

Ah okay, currently the init stuff is in the Service class so I modelled this one after that.

Would we want to add just systemctl to the generic module for now so we don't have to find the places that are currently using LinuxAdmin::Service or is there a better way to do that so we don't break things using the current implementation of Service?

@carbonin
Copy link
Member Author

Oh I guess subclassing wouldn't change the namespace right? So we would probably be fine there. Forgive my ruby inexperience.

@carbonin
Copy link
Member Author

@Fryguy is the idea to have an abstract class as a parent of all service management tool classes or something more like a proxy to determine which class to use based on what distro/version we are currently running on?

@carbonin
Copy link
Member Author

I could use the existence of the systemctl command to determine when I would use that over the service command.

Copy link
Member

Choose a reason for hiding this comment

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

I like name instead of id.

@gtanzillo
Copy link
Member

@Fryguy Discussed this with @carbonin, @jrafanie and we thought is would be best to merge this PR now so and work on the generic class separately. This way we get the systemctl support now and the new can be done on top of that.

@jrafanie
Copy link
Member

Yeah, my thoughts were that we'd be changing the interface of the existing Service class to make it a subclass so let's get this PR in since it's strictly an unbreaking change. We can tag it if need be, then we can do minor rework of the Service management classes to create a proxy base class that instantiates the subclasses under the covers.

@Fryguy
Copy link
Member

Fryguy commented Jul 24, 2015

is the idea to have an abstract class as a parent of all service management tool classes or something more like a proxy to determine which class to use based on what distro/version we are currently running on?

A little bit of both. See how we did Package (which has Rpm and Yum (and potentially Apt or whatver) as subclasses), or how we did RegistrationSystem (which has Rhn or SubscriptionManager as subclasses).

Basically, a user could just say something like RegistrationSystem.update and it would do the right thing once it detects what's on the system. If the subclasses implement the same methods, it's completely pluggable.

so let's get this PR in since it's strictly an unbreaking change

Not really. Adding a new class changes the public API, which means at least a minor version release. Then when you change it again to remove it or renamespace it, it's another minor version release.

If you guys just want something in for testing, you can use a SHA (no need for a tag), or even just use the reference to the branch on your fork.

@gtanzillo
Copy link
Member

@Fryguy We were planning to continue to support the new class after we add the new generic one. This way can use systemctl directly if someone needed to do that.

@jrafanie
Copy link
Member

Not really. Adding a new class changes the public API, which means at least a minor version release. Then when you change it again to remove it or renamespace it, it's another minor version release.

I was more thinking that we may have to rename things to make sense since Service seems like a great name for the generic interface class that the sysvinit vs. systemd classes would subclass. Either way, it seems like a follow up PR.

@miq-bot
Copy link
Member

miq-bot commented Jul 24, 2015

Checked commits carbonin@88a2a3c .. carbonin@6894857 with rubocop 0.32.0
3 files checked, 0 offenses detected
Everything looks good. 👍

@Fryguy
Copy link
Member

Fryguy commented Jul 24, 2015

We were planning to continue to support the new class after we add the new generic one. This way can use systemctl directly if someone needed to do that.

Oh I see, it implements the same methods too, so it's good. Even so, we try to not to use the specific subclass directly, instead opting for the generic interface.

So, yeah, this is good to merge. Honestly though, creating the matching interface it probably like 30 minutes worth of work.

Fryguy added a commit that referenced this pull request Jul 24, 2015
Added a systemctl class to the linux admin gem
@Fryguy Fryguy merged commit edb47c8 into ManageIQ:master Jul 24, 2015
@carbonin carbonin deleted the add_systemctl_class branch July 24, 2015 18:55
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.

6 participants