Skip to content

Make lv that is a substring of a vg work #244

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

Merged
merged 1 commit into from
Feb 5, 2020

Conversation

genebean
Copy link
Contributor

This PR builds on #230. In local testing all tests related to this PR pass while some facter-related tests failed on this branch and on master.

Prior to this commit a logical volume that was a substring of the name of a volume group could not be created due to a false positive by the exists? method of the provider.

The sample output of lvs in the spec tests was updated to this:

  LV      VG         Attr       LSize   Pool Origin Data%  Meta%  Move Log Cpy%Sync Convert
  lv_root VolGroup   -wi-ao----  18.54g
  lv_swap VolGroup   -wi-ao---- 992.00m
  data    data       -wi-ao---- 992.00m
  j1      vg_jenkins -wi-a-----   1.00g

After adding a test to verify that a logical volume named jenkins does not exist the original exists? check failed like so:

Puppet::Type::Logical_volume::ProviderLvm
  self.instances
    returns an array of logical volumes
  when checking existence
    returns 'true', lv 'data' in vg 'data' exists
    returns 'nil', lv 'jenkins' in vg 'vg_jenkins' exists (FAILED - 5)
    returns 'nil', lv 'data' in vg 'myvg' does not exist

After updating the exists? method to instead use lvs_pattern the new tests passes:

Puppet::Type::Logical_volume::ProviderLvm
  self.instances
    returns an array of logical volumes
  when checking existence
    returns 'true', lv 'data' in vg 'data' exists
    returns 'nil', lv 'jenkins' in vg 'vg_jenkins' exists
    returns 'nil', lv 'data' in vg 'myvg' does not exist

Co-authored-by: Ruediger Pluem [email protected]

Prior to this commit a logical volume that was a substring of the name of a volume group could not be created due to a false positive by the `exists?` method of the provider.

The sample output of `lvs` in the spec tests was updated to this:

```
  LV      VG         Attr       LSize   Pool Origin Data%  Meta%  Move Log Cpy%Sync Convert
  lv_root VolGroup   -wi-ao----  18.54g
  lv_swap VolGroup   -wi-ao---- 992.00m
  data    data       -wi-ao---- 992.00m
  j1      vg_jenkins -wi-a-----   1.00g
```

After adding a test to verify that a logical volume named `jenkins` does not exist the original `exists?` check failed like so:

```
Puppet::Type::Logical_volume::ProviderLvm
  self.instances
    returns an array of logical volumes
  when checking existence
    returns 'true', lv 'data' in vg 'data' exists
    returns 'nil', lv 'jenkins' in vg 'vg_jenkins' exists (FAILED - 5)
    returns 'nil', lv 'data' in vg 'myvg' does not exist
```

After updating the `exists?` method to instead use `lvs_pattern` the new tests passes:

```
Puppet::Type::Logical_volume::ProviderLvm
  self.instances
    returns an array of logical volumes
  when checking existence
    returns 'true', lv 'data' in vg 'data' exists
    returns 'nil', lv 'jenkins' in vg 'vg_jenkins' exists
    returns 'nil', lv 'data' in vg 'myvg' does not exist
```

Co-authored-by: Ruediger Pluem <[email protected]>
@codecov-io
Copy link

codecov-io commented Jan 20, 2020

Codecov Report

Merging #244 into master will increase coverage by 0.12%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #244      +/-   ##
==========================================
+ Coverage    72.7%   72.83%   +0.12%     
==========================================
  Files          17       17              
  Lines         773      773              
==========================================
+ Hits          562      563       +1     
+ Misses        211      210       -1
Impacted Files Coverage Δ
lib/puppet/provider/logical_volume/lvm.rb 69.36% <100%> (+0.57%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d6c3514...d8c31dd. Read the comment docs.

@sheenaajay
Copy link
Contributor

sheenaajay commented Jan 20, 2020

@genebean Could you please confirm if the following changes are causing new failures (facter-related tests)on master branch and this branch or are they existing failures.

" In local testing all tests related to this PR pass while some facter-related tests failed on this branch and on master."

@genebean
Copy link
Contributor Author

The same errors are present without these changes. You can confirm by cloning, checking out master, and running pdk test unit

Sent with GitHawk

@vStone
Copy link

vStone commented Jan 21, 2020

I have tested this using pdk and using plain bundle rake and I can confirm that all tests run.

@sheenaajay
Copy link
Contributor

Thanks alot @vStone @genebean

@sheenaajay
Copy link
Contributor

@genebean @vStone Executed pdk test unit locally on this branch and master branch both gave failures on the following tests.
Looks like its not introduced with this PR. Existing failures on master branch.
rspec ./spec/unit/facter/lvm_support_spec.rb:54 # lvm_vgs facts when there is lvm support when there are no vgs is set to 0
rspec ./spec/unit/facter/lvm_support_spec.rb:63 # lvm_vgs facts when there is lvm support when there are vgs lists vgs
rspec ./spec/unit/facter/lvm_support_spec.rb:94 # lvm_pvs facts when there is lvm support when there are no pvs is set to 0
rspec ./spec/unit/facter/lvm_support_spec.rb:103 # lvm_pvs facts when there is lvm support when there are pvs lists pvs

@rpluem
Copy link
Contributor

rpluem commented Jan 30, 2020

Anything I can provide to move this forward?

@DavidS DavidS merged commit 69ab182 into puppetlabs:master Feb 5, 2020
@DavidS
Copy link
Contributor

DavidS commented Feb 5, 2020

Thanks everyone for your contributions!

@rpluem
Copy link
Contributor

rpluem commented Feb 6, 2020

@DavidS Thanks. Two questions though:

  1. What about the test in d64c11d? Shouldn't it be added as well?
  2. What can I do to get this included in a release?

@genebean
Copy link
Contributor Author

genebean commented Feb 6, 2020

@rpluem Regarding the test, I'd suggest a new PR that just adds that.

@genebean genebean deleted the fix_substing_false_positive branch February 6, 2020 12:57
rpluem pushed a commit to rpluem/puppetlabs-lvm that referenced this pull request Feb 6, 2020
Test that exists returns nil if the name of the logical volume is a
substring of the name of an existing logical volume.

(cherry picked from commit d64c11d)
@rpluem
Copy link
Contributor

rpluem commented Feb 6, 2020

@rpluem Regarding the test, I'd suggest a new PR that just adds that.

@genebean Done as #246

DavidS added a commit that referenced this pull request Feb 6, 2020
@rpluem
Copy link
Contributor

rpluem commented Feb 11, 2020

@DavidS, @genebean What can / should / must I do to get this included in a release?

@genebean
Copy link
Contributor Author

I’m going to try and do the release when I go in today. I had some tooling problems on my laptop that kept me from doing it Friday or yesterday that should be resolved now.

Sent with GitHawk

cegeka-jenkins pushed a commit to cegeka/puppet-lvm that referenced this pull request Jun 4, 2020
Test that exists returns nil if the name of the logical volume is a
substring of the name of an existing logical volume.

(cherry picked from commit d64c11d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants