-
Notifications
You must be signed in to change notification settings - Fork 683
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
Deprecate and warn when comparing against OS name with capitals/spaces #2397
Conversation
80d8a54
to
d4ab29d
Compare
d4ab29d
to
68c2f9f
Compare
@adamleff - I think this is ready for review, it will want to go in tandem with the train release. inspec/train#228 Note: I will redo this to be off the platform resource once that gets merged in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a couple of unit tests for NameCleaned
? I'm thinking something for when translation was necessary, and something for when it wasn't? Just some checks using the ==
method you define should be good.
lib/resources/os.rb
Outdated
cleaned = other.downcase.tr(' ', '_') | ||
Inspec::Log.warn "[DEPRECATED] Platform names will become lowercase in InSpec 2.0. Please match on '#{cleaned}' instead of '#{other}'" | ||
end | ||
super(other) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to compare against cleaned
if we had to translate, right?
lib/resources/os.rb
Outdated
def [](name) | ||
# convert string to symbol | ||
name = name.to_sym if name.is_a? String | ||
return self.name if name == :name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For clarity purposes, we should probably change the method parameter from name
to key
or something like that so we're not overloading the use of name
here.
test/unit/resources/os_test.rb
Outdated
@@ -6,6 +6,24 @@ | |||
require 'inspec/resource' | |||
|
|||
describe 'Inspec::Resources::Os' do | |||
it 'verify NamedCleaned with uppercase' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgive the pun, but can we Clean
this up a bit?
let(:cleaner) { Inspec::Resources::OSResource::NameCleaned }
it 'does something' do
cleaner.new('blah')
# some test here
end
It feels repetitious to be defining cleaner
in each test like that. If you don't want to use a let
, we should at least consolidate two lines per test:
it 'does something' do
name = Inspec::Resources::OSResource::NameCleaned.new('something')
# the test
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conditionally approved pending a train release and a commit here that uses it.
Signed-off-by: Jared Quick <jquick@chef.io>
Signed-off-by: Jared Quick <jquick@chef.io>
Signed-off-by: Jared Quick <jquick@chef.io>
bfff11c
to
ba04e38
Compare
Signed-off-by: Jared Quick <jquick@chef.io>
ba04e38
to
e45e634
Compare
This change supports the new train lowercase/underscored platform names. inspec/train#228
Fixes #2407
This change will auto convert any profiles using the wrong format and give a warning to fix by 2.0.
Signed-off-by: Jared Quick jquick@chef.io