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

Reorganize Yast2 service classes #799

Merged
merged 10 commits into from Aug 23, 2018

Conversation

@coveralls

This comment has been minimized.

Copy link

commented Aug 14, 2018

Coverage Status

Coverage increased (+0.03%) to 29.807% when pulling 8823245 on feature/reorganize-yast2-service-classes into 3586c81 on master.

@dgdavid dgdavid force-pushed the feature/reorganize-yast2-service-classes branch 3 times, most recently from 76597a4 to 987fede Aug 14, 2018

@imobachgs
Copy link
Contributor

left a comment

Thanks a lot for this refactor! I've added some comments. And, when you are done, update the changes file and bump the version number, please.

require "yast2/systemd/socket"

# Systemd control API
module Systemd; end

This comment has been minimized.

Copy link
@imobachgs

imobachgs Aug 20, 2018

Contributor

Should not be Yast2::Systemd?

end
end

# Systemd.service unit control API

This comment has been minimized.

Copy link
@imobachgs

imobachgs Aug 20, 2018

Contributor

Yast2::Systemd

# ## Get a service unit by its name
# ## If the service unit can't be found, you'll get nil
#
# service = Yast::Systemd::Service.find('sshd') # service unit object

This comment has been minimized.

Copy link
@imobachgs

imobachgs Aug 20, 2018

Contributor

It should be Yast2::Systemd::Service. Please, check other occurrences in this documentation.


module Yast2
module Systemd
# Represent a missed service

This comment has been minimized.

Copy link
@imobachgs

imobachgs Aug 20, 2018

Contributor

I would say missing.

# @param service_name [String] "foo" or "foo.service"
# @param propmap [Systemd::Unit::PropMap]
# @return [Service,nil] `nil` if not found
def find(service_name, propmap = {})

This comment has been minimized.

Copy link
@imobachgs

imobachgs Aug 20, 2018

Contributor

It looks strange that propmap is expected to be an instance of Yast2::Systemd::Unit::Propmap but the default value is a hash.

This comment has been minimized.

Copy link
@dgdavid

dgdavid Aug 20, 2018

Author Member

I agree. Shall we change it now? WDYT?

This comment has been minimized.

Copy link
@imobachgs

imobachgs Aug 21, 2018

Contributor

No, as a Propmap is basically a Hash, let's keep it out of the scope of this PR.

This comment has been minimized.


# @param propmap [SystemdUnit::PropMap]
def all(propmap = {})
sockets = Systemctl.socket_units.map { |socket_unit| new(socket_unit, propmap) }

This comment has been minimized.

Copy link
@imobachgs

imobachgs Aug 20, 2018

Contributor

Please, use a 1-letter identifier.

#
# @example Create a systemd service unit
#
# class Service < Yast::Systemd::Unit

This comment has been minimized.

Copy link
@imobachgs

imobachgs Aug 20, 2018

Contributor

Yast2::Systemd::Unit

end

# Structure holding properties of systemd unit
class Properties < OpenStruct

This comment has been minimized.

Copy link
@imobachgs

imobachgs Aug 20, 2018

Contributor

What about moving this class to its own file?

else
# Check for exit code of `systemctl is-enabled systemd_unit.name` ; additionally
# test the stdout of the command for valid values when the service is enabled
# http://www.freedesktop.org/software/systemd/man/systemctl.html#is-enabled%20NAME...

This comment has been minimized.

Copy link
@imobachgs

imobachgs Aug 20, 2018

Contributor

The correct link has changed to http://www.freedesktop.org/software/systemd/man/systemctl.html#is-enabled UNIT… (not nice anchor name IMHO).

#
# Once the inst-sys has running dbus/systemd, this class definition can be removed
# together with the condition for Stage.initial in the Systemd::Unit#show.
class InstallationProperties < OpenStruct

This comment has been minimized.

Copy link
@imobachgs

imobachgs Aug 20, 2018

Contributor

Please, move this into its own file too.

@dgdavid dgdavid force-pushed the feature/reorganize-yast2-service-classes branch 3 times, most recently from 43f8d8a to 00c3fb7 Aug 21, 2018

Missing changes entry (and version bump)

@dgdavid

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2018

You're right @imobachgs

Thank you

@imobachgs
Copy link
Contributor

left a comment

LGTM. Just merge after having the PRs for the affected packages approved :)

@dgdavid dgdavid force-pushed the feature/reorganize-yast2-service-classes branch from 5861e57 to e20e19f Aug 21, 2018

dgdavid added 10 commits Aug 14, 2018

@dgdavid dgdavid force-pushed the feature/reorganize-yast2-service-classes branch from 60a2576 to 8823245 Aug 22, 2018

@dgdavid dgdavid merged commit e4ab100 into master Aug 23, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 29.807%
Details

@dgdavid dgdavid deleted the feature/reorganize-yast2-service-classes branch Aug 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.