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

filesystem: Add free_kb, size_kb, type, and percent_free properties #3778

Merged
merged 8 commits into from Feb 6, 2019

Conversation

Projects
None yet
4 participants
@clintoncwolfe
Copy link
Contributor

clintoncwolfe commented Feb 4, 2019

This PR is an adoption of #3767.

In this PR:

  • A serious bug is found in the size property; on linux it reports in KB, on Windows it reports in GB; KB was the documented unit. As existing tests may rely on that behavior, the size property is scheduled for future deprecation. A warning is added to the docs.
  • size_kb is introduced, with guaranteed units.
  • The free property from #3767 is renamed free_kb and its units corrected.
  • percent_free is added, range 0..100
  • The type property introduced on #3767 is retained without changes
  • The unit tests are tightened to test the actual value returned, rather than simply >= 1 (which allowed the GB/KB issue on windows to slip through).

Many thanks to @jmassardo for the original work on this to add free and type, and to @miah for finding the pre-existing issues with units.

@jerryaldrichiii
Copy link
Contributor

jerryaldrichiii left a comment

Looks good to me. Just a few comments.

end

<br>

### Test if the C:\ partition is NTFS

describe filesystem('c:\') do

This comment has been minimized.

@jerryaldrichiii

jerryaldrichiii Feb 4, 2019

Contributor

Probably doesn't matter, but I'd capitalize c here.

This comment has been minimized.

@miah

miah Feb 6, 2019

Contributor

Windows drive letters are case insensitive, capital might make it stand out a little in docs but its not a requirement. <3

its('size_kb') { should be >= 32000 }
its('free_kb') { should be >= 3200 }
its('type') { should cmp 'ext4' }
its('percent_free') { should be >= 20 }
end
describe filesystem('c:') do

This comment has been minimized.

@jerryaldrichiii

jerryaldrichiii Feb 4, 2019

Contributor

Ah, well this is lowercase and no slash. I think we should standardize but don't care enough to hold a PR over it.

This comment has been minimized.

@miah

miah Feb 6, 2019

Contributor

Agree that we should at least be consistent.

@jmassardo

This comment has been minimized.

Copy link
Contributor

jmassardo commented Feb 5, 2019

@clintoncwolfe this looks awesome! Thanks for taking across the finish line for us!

jmassardo and others added some commits Jan 29, 2019

Add free space and type to filesystem resource
Signed-off-by: James Massardo <jmassardo@chef.io>
update helpers and mocks so tests run successfully
Signed-off-by: James Massardo <jmassardo@chef.io>
removed unneeded library
Signed-off-by: James Massardo <jmassardo@chef.io>
missed rubocop error
Signed-off-by: James Massardo <jmassardo@chef.io>
Add percent_free property
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
Add size_kb, and correct Powershell code to return KB for both total …
…size and free space, rather than total in GB and free space in bytes

Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
Rename free to free_kb
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>
Add deprecation hook for size()
Signed-off-by: Clinton Wolfe <clintoncwolfe@gmail.com>

@clintoncwolfe clintoncwolfe force-pushed the team/filesystem-free-percent branch from 95bd8a8 to dd3d3cb Feb 6, 2019

@miah

miah approved these changes Feb 6, 2019

Copy link
Contributor

miah left a comment

its('size_kb') { should be >= 32000 }
its('free_kb') { should be >= 3200 }
its('type') { should cmp 'ext4' }
its('percent_free') { should be >= 20 }
end
describe filesystem('c:') do

This comment has been minimized.

@miah

miah Feb 6, 2019

Contributor

Agree that we should at least be consistent.

@clintoncwolfe clintoncwolfe merged commit 48711fa into master Feb 6, 2019

4 checks passed

DCO This commit has a DCO Signed-off-by
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
expeditor/config-validation Validated your Expeditor config file
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment