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

Provide service params #721

Merged
merged 1 commit into from
May 9, 2016
Merged

Provide service params #721

merged 1 commit into from
May 9, 2016

Conversation

alexpop
Copy link
Contributor

@alexpop alexpop commented May 6, 2016

Replacing the current method of accessing these params:

describe service('avahi-daemon').info[:properties]['UnitFileState'] do
  it { should be 'enabled' }
end

with this:

  # This test will succeed if the service is not enabled
  describe service('avahi-daemon').params do
    its('UnitFileState') { should_not eq 'enabled' }
  end

  # This test will fail with expected: "enabled", got nil
  describe service('MissingInActionService').params do
    its('MissingInActionParam') { should eq 'enabled' }
  end
  1. Not happy with the hashie dependency, but wanted to share the vision and start the discussion.
  2. Should we make the info method private now?
  3. params looks to be an internal inspec way of handing attributes. Is properties a better name for this method? For example:
  describe service('avahi-daemon').properties do
    its('UnitFileState') { should_not eq 'enabled' }
  end

@alexpop alexpop added the Type: Enhancement Improves an existing feature label May 6, 2016
@arlimus
Copy link
Contributor

arlimus commented May 6, 2016

+1 in making info private; is there anything in info, that is not exposed via accessors? anything that shouldn't be exposed via accessors but only via info?

@alexpop
Copy link
Contributor Author

alexpop commented May 6, 2016

inspec> s.info
=> {:name=>"sshd.service",
 :description=>"OpenSSH server daemon",
 :installed=>true,
 :running=>true,
 :enabled=>true,
 :type=>"systemd",
 :params=>
  {"Type"=>"simple",
   "Restart"=>"on-failure",

Right now, out of this bunch, only installed, running and enabled are available directly. Missing name, description and type

@chris-rock
Copy link
Contributor

I like the service('avahi-daemon').params approach because it works in harmony with other resources we have. We could easily have:

 describe service('sshd')do
    its('name') { should eq 'sshd.service' }
   its('description') { should eq 'OpenSSH server daemon' }
   its('type') { should eq 'systemd' }
 end

@alexpop alexpop changed the title WIP: Provide service params Provide service params May 9, 2016
@alexpop
Copy link
Contributor Author

alexpop commented May 9, 2016

Ok, with the latest code, this is working now:

  describe service('avahi-daemon').params do
    its('UnitFileState') { should_not eq 'enabled' }
  end

  describe service('sshd') do
    its('type') { should eq 'systemd' }
    its('name') { should eq 'sshd.service' }
    its('description') { should eq 'OpenSSH server daemon' }
    its('params.UnitFileState') { should eq 'enabled' }
  end

@@ -22,7 +24,9 @@
it 'verify ubuntu package parsing' do
resource = MockLoader.new(:ubuntu1404).load_resource('service', 'ssh')
srv = { name: 'ssh', description: nil, installed: true, running: true, enabled: true, type: 'upstart' }
Copy link
Contributor

Choose a reason for hiding this comment

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

can we remove the srv?

@chris-rock
Copy link
Contributor

@alexpop Implementation is very good. I would like to remove all srv code, where we replaced it with specific test. Also I would add the .params tests for all systemd tests (those have been covered by the info object before):

 describe service('avahi-daemon').params do
    its('UnitFileState') { should_not eq 'enabled' }
  end

_(resource.installed?).must_equal true
_(resource.enabled?).must_equal true
_(resource.running?).must_equal true
end

it 'verify centos 7 package parsing with systemd_service and service_ctl override' do
resource = MockLoader.new(:centos7).load_resource('systemd_service', 'sshd', '/path/to/systemctl')
srv = { name: 'sshd.service', description: 'OpenSSH server daemon', installed: true, running: true, enabled: true, type: 'systemd', :properties=>{"Id"=>"sshd.service", "Names"=>"sshd.service", "Description"=>"OpenSSH server daemon", "LoadState"=>"loaded", "UnitFileState"=>"enabled", "SubState"=>"running"} }
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi Alex, would it make sense to re-add the systemd tests:

_(resource.params).must_equal {object}'

@chris-rock
Copy link
Contributor

Awesome addition @alexpop

@chris-rock chris-rock merged commit 341ebee into master May 9, 2016
@chris-rock chris-rock deleted the ap/service-params branch May 9, 2016 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement Improves an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants