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

Support multiple properties with be_zfs matcher and fix edge case #41

Merged
merged 2 commits into from Apr 9, 2013

Conversation

Projects
None yet
2 participants
@mizzy
Owner

mizzy commented Apr 8, 2013

I've fixed the edge case mentioned in #40 .

And also I've added a feature that be_zfs.with takes multple properties like this.

it do
  should be_zfs.with(
    'mountpoint'  => '/rpool',
    'compression' => 'off'
  )
end

@ftnk , I appreciate if you check them.

@mizzy mizzy referenced this pull request Apr 8, 2013

Merged

add zfs matcher #40

@ftnk

This comment has been minimized.

Show comment
Hide comment
@ftnk

ftnk Apr 8, 2013

Contributor

Supporting multiple property is good idea and
it works.

But with multi property, Failure/Error output does not make sense.

Failures:

  1. rpool
    Failure/Error: })
    expected "rpool" to be zfs

    ./spec/localhost/zfs.rb:10:in `block (2 levels) in <top (required)>'

with this output., I can't find out which property is wrong.

2013/4/8 Gosuke Miyashita notifications@github.com:

I've fixed the edge case mentioned in #40 .

And also I've added a feature that be_zfs.with takes multple properties like
this.

it do
should be_zfs.with({
'mountpoint' => '/rpool',
'compression' => 'off',
})
end

@ftnk , I appreciate if you check them.


You can merge this Pull Request by running

git pull https://github.com/mizzy/serverspec
multiple_properties_with_be_zfs_matcher

Or view, comment on, or merge it at:

#41

Commit Summary

Support multiple properties with be_zfs matcher and fix edge case

File Changes

M lib/serverspec/commands/solaris.rb (7)
M spec/solaris/commads_spec.rb (15)

Patch Links:

https://github.com/mizzy/serverspec/pull/41.patch
https://github.com/mizzy/serverspec/pull/41.diff

Contributor

ftnk commented Apr 8, 2013

Supporting multiple property is good idea and
it works.

But with multi property, Failure/Error output does not make sense.

Failures:

  1. rpool
    Failure/Error: })
    expected "rpool" to be zfs

    ./spec/localhost/zfs.rb:10:in `block (2 levels) in <top (required)>'

with this output., I can't find out which property is wrong.

2013/4/8 Gosuke Miyashita notifications@github.com:

I've fixed the edge case mentioned in #40 .

And also I've added a feature that be_zfs.with takes multple properties like
this.

it do
should be_zfs.with({
'mountpoint' => '/rpool',
'compression' => 'off',
})
end

@ftnk , I appreciate if you check them.


You can merge this Pull Request by running

git pull https://github.com/mizzy/serverspec
multiple_properties_with_be_zfs_matcher

Or view, comment on, or merge it at:

#41

Commit Summary

Support multiple properties with be_zfs matcher and fix edge case

File Changes

M lib/serverspec/commands/solaris.rb (7)
M spec/solaris/commads_spec.rb (15)

Patch Links:

https://github.com/mizzy/serverspec/pull/41.patch
https://github.com/mizzy/serverspec/pull/41.diff

@ftnk

This comment has been minimized.

Show comment
Hide comment
@ftnk

ftnk Apr 8, 2013

Contributor

About edge case mentioned in #40

I'm sorry for writing wrong output of "zfs get -H compression rpool".
Correctly, output is tab separated.

And "/sbin/zfs list -H -o #{key} #{zfs}" returns value only.

% zfs list -H -o compression rpool
off

I think "/sbin/zfs list -H -o #{key} #{zfs} | grep ^#{value}$" is
better than "/sbin/zfs get -H #{key} #{zfs} | grep ' #{value} '".

2013/4/8 Gosuke Miyashita notifications@github.com:

I've fixed the edge case mentioned in #40 .

And also I've added a feature that be_zfs.with takes multple properties like
this.

it do
should be_zfs.with({
'mountpoint' => '/rpool',
'compression' => 'off',
})
end

@ftnk , I appreciate if you check them.


You can merge this Pull Request by running

git pull https://github.com/mizzy/serverspec
multiple_properties_with_be_zfs_matcher

Or view, comment on, or merge it at:

#41

Commit Summary

Support multiple properties with be_zfs matcher and fix edge case

File Changes

M lib/serverspec/commands/solaris.rb (7)
M spec/solaris/commads_spec.rb (15)

Patch Links:

https://github.com/mizzy/serverspec/pull/41.patch
https://github.com/mizzy/serverspec/pull/41.diff

Contributor

ftnk commented Apr 8, 2013

About edge case mentioned in #40

I'm sorry for writing wrong output of "zfs get -H compression rpool".
Correctly, output is tab separated.

And "/sbin/zfs list -H -o #{key} #{zfs}" returns value only.

% zfs list -H -o compression rpool
off

I think "/sbin/zfs list -H -o #{key} #{zfs} | grep ^#{value}$" is
better than "/sbin/zfs get -H #{key} #{zfs} | grep ' #{value} '".

2013/4/8 Gosuke Miyashita notifications@github.com:

I've fixed the edge case mentioned in #40 .

And also I've added a feature that be_zfs.with takes multple properties like
this.

it do
should be_zfs.with({
'mountpoint' => '/rpool',
'compression' => 'off',
})
end

@ftnk , I appreciate if you check them.


You can merge this Pull Request by running

git pull https://github.com/mizzy/serverspec
multiple_properties_with_be_zfs_matcher

Or view, comment on, or merge it at:

#41

Commit Summary

Support multiple properties with be_zfs matcher and fix edge case

File Changes

M lib/serverspec/commands/solaris.rb (7)
M spec/solaris/commads_spec.rb (15)

Patch Links:

https://github.com/mizzy/serverspec/pull/41.patch
https://github.com/mizzy/serverspec/pull/41.diff

@mizzy

This comment has been minimized.

Show comment
Hide comment
@mizzy

mizzy Apr 8, 2013

Owner

Failures:

  1. rpool
    Failure/Error: })
    expected "rpool" to be zfs

    ./spec/localhost/zfs.rb:10:in `block (2 levels) in <top (required)>'

Please paste the content of ./spec/localhost/zfs.rb.

And I will fix the command "/sbin/zfs get -H #{key} #{zfs} | grep ' #{value} '"
to "/sbin/zfs list -H -o #{key} #{zfs} | grep ^#{value}$". Thanks.

Owner

mizzy commented Apr 8, 2013

Failures:

  1. rpool
    Failure/Error: })
    expected "rpool" to be zfs

    ./spec/localhost/zfs.rb:10:in `block (2 levels) in <top (required)>'

Please paste the content of ./spec/localhost/zfs.rb.

And I will fix the command "/sbin/zfs get -H #{key} #{zfs} | grep ' #{value} '"
to "/sbin/zfs list -H -o #{key} #{zfs} | grep ^#{value}$". Thanks.

@mizzy

This comment has been minimized.

Show comment
Hide comment
@mizzy

mizzy Apr 8, 2013

Owner

I've fixed the command for exact match.Please check it.

Owner

mizzy commented Apr 8, 2013

I've fixed the command for exact match.Please check it.

@ftnk

This comment has been minimized.

Show comment
Hide comment
@ftnk

ftnk Apr 8, 2013

Contributor

./spec/localost/zfs.rb

require 'spec_helper'

describe 'rpool' do
  it { should be_zfs }

  it do
    should be_zfs.with({
      'mountpoint'  => '/rpool',
      'compression' => 'on',
    })
  end
end

And real values are as below

% zfs list -H -o mountpoint rpool
/rpool
% zfs list -H -o compression rpool
off

test ./spec/localost/zfs.rb

% ruby19 -S rspec ./spec/localhost/zfs.rb
.F

Failures:

  1) rpool
     Failure/Error: })
       expected "rpool" to be zfs
     # ./spec/localhost/zfs.rb:10:in `block (2 levels) in <top

(required)>'

Finished in 0.04138 seconds
2 examples, 1 failure

Failed examples:

rspec ./spec/localhost/zfs.rb:6 # rpool
Contributor

ftnk commented Apr 8, 2013

./spec/localost/zfs.rb

require 'spec_helper'

describe 'rpool' do
  it { should be_zfs }

  it do
    should be_zfs.with({
      'mountpoint'  => '/rpool',
      'compression' => 'on',
    })
  end
end

And real values are as below

% zfs list -H -o mountpoint rpool
/rpool
% zfs list -H -o compression rpool
off

test ./spec/localost/zfs.rb

% ruby19 -S rspec ./spec/localhost/zfs.rb
.F

Failures:

  1) rpool
     Failure/Error: })
       expected "rpool" to be zfs
     # ./spec/localhost/zfs.rb:10:in `block (2 levels) in <top

(required)>'

Finished in 0.04138 seconds
2 examples, 1 failure

Failed examples:

rspec ./spec/localhost/zfs.rb:6 # rpool
@ftnk

This comment has been minimized.

Show comment
Hide comment
@ftnk

ftnk Apr 9, 2013

Contributor

Thanks. It works fine!

2013/4/9 Gosuke Miyashita notifications@github.com

I've fixed the command for exact match.Please check it.


Reply to this email directly or view it on GitHubhttps://github.com/mizzy/serverspec/pull/41#issuecomment-16086442
.

Contributor

ftnk commented Apr 9, 2013

Thanks. It works fine!

2013/4/9 Gosuke Miyashita notifications@github.com

I've fixed the command for exact match.Please check it.


Reply to this email directly or view it on GitHubhttps://github.com/mizzy/serverspec/pull/41#issuecomment-16086442
.

@mizzy

This comment has been minimized.

Show comment
Hide comment
@mizzy

mizzy Apr 9, 2013

Owner

Do multiple properties work fine? Or Only does command fixing work?

Owner

mizzy commented Apr 9, 2013

Do multiple properties work fine? Or Only does command fixing work?

@ftnk

This comment has been minimized.

Show comment
Hide comment
@ftnk

ftnk Apr 9, 2013

Contributor

Both of them work.

Problem about multiple properties is result output.

test below

it { should be_zfs.with({ 'compression' => 'on' }) }

return like below

  1) rpool
     Failure/Error: it { should be_zfs.with({ 'compression' => 'on' }) }

I can find out value of 'compression' is wrong.

test multiple properies

  it do
    should be_zfs.with({
      'mountpoint'  => '/rpool',
      'compression' => 'on',
    })
  end

return like below.

  1) rpool
     Failure/Error: })
       expected "rpool" to be zfs

With this output, I can't find out which value is wrong, mountpoint?
compression?
And I need to check each value.

Single property is better for me.

2013/4/9 Gosuke Miyashita notifications@github.com

Do multiple properties work fine? Or Only does command fixing work?


Reply to this email directly or view it on GitHubhttps://github.com/mizzy/serverspec/pull/41#issuecomment-16086843
.

Contributor

ftnk commented Apr 9, 2013

Both of them work.

Problem about multiple properties is result output.

test below

it { should be_zfs.with({ 'compression' => 'on' }) }

return like below

  1) rpool
     Failure/Error: it { should be_zfs.with({ 'compression' => 'on' }) }

I can find out value of 'compression' is wrong.

test multiple properies

  it do
    should be_zfs.with({
      'mountpoint'  => '/rpool',
      'compression' => 'on',
    })
  end

return like below.

  1) rpool
     Failure/Error: })
       expected "rpool" to be zfs

With this output, I can't find out which value is wrong, mountpoint?
compression?
And I need to check each value.

Single property is better for me.

2013/4/9 Gosuke Miyashita notifications@github.com

Do multiple properties work fine? Or Only does command fixing work?


Reply to this email directly or view it on GitHubhttps://github.com/mizzy/serverspec/pull/41#issuecomment-16086843
.

@mizzy

This comment has been minimized.

Show comment
Hide comment
@mizzy

mizzy Apr 9, 2013

Owner

I see what you point.I will merge to master.

If you have any idea to find which property is wrong with multiple properties, please tell me.

Thanks.

Owner

mizzy commented Apr 9, 2013

I see what you point.I will merge to master.

If you have any idea to find which property is wrong with multiple properties, please tell me.

Thanks.

@mizzy

This comment has been minimized.

Show comment
Hide comment
@mizzy

mizzy Apr 9, 2013

Owner

You can write the spec without { and } like this.

it { should be_zfs.with( 'compression' => 'on' ) }

Or

it do
  should be_zfs.with(
    'mountpoint'  => '/rpool',
    'compression' => 'on'
  )
end
Owner

mizzy commented Apr 9, 2013

You can write the spec without { and } like this.

it { should be_zfs.with( 'compression' => 'on' ) }

Or

it do
  should be_zfs.with(
    'mountpoint'  => '/rpool',
    'compression' => 'on'
  )
end

mizzy added a commit that referenced this pull request Apr 9, 2013

Merge pull request #41 from mizzy/multiple_properties_with_be_zfs_mat…
…cher

Support multiple properties with be_zfs matcher and fix edge case

@mizzy mizzy merged commit 553dcb7 into master Apr 9, 2013

1 check passed

default The Travis build passed
Details

@mizzy mizzy deleted the multiple_properties_with_be_zfs_matcher branch Apr 9, 2013

@mizzy

This comment has been minimized.

Show comment
Hide comment
@mizzy

mizzy Apr 9, 2013

Owner

Merged and release as v0.2.2. Thanks!

Owner

mizzy commented Apr 9, 2013

Merged and release as v0.2.2. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment