Handle more multipackage scenarios #790
Conversation
Detect when blocks that include more than just the package install. It's pretty common that they also include an apt_update or an include_recipe 'apt' when we're on a debian-ish platform. I plan to open another PR to refactor the if detection with this same support. baby steps here. Signed-off-by: Tim Smith <tsmith@chef.io>
|
Fixes #789 |
This caused 14 crashes on supermarket Signed-off-by: Tim Smith <tsmith@chef.io>
| expect_no_offenses(<<~RUBY) | ||
| case node['platform_family'] | ||
| when /debian/ | ||
| package 'it-is-fine' |
There was a problem hiding this comment.
should this test theoretically trigger the rule, in order to be a valid test? LIke:
when /debian/
%w(bmon htop).each { |p| package p }
?
There was a problem hiding this comment.
At the moment we can't handle the regex, but we need to make sure we don't crash.
There was a problem hiding this comment.
Sure but it seems like we should not-crash in both cases, right? When the package rule is good and bad (regardless of if we do/don't catch the error)
There was a problem hiding this comment.
Yeah we were crashing before so I added code to not crash on regexes and added a test. Eventually we need another rule to convert unnecessary regexes in case statements, but until that happens we need to just skip these.
There was a problem hiding this comment.
I get that. I'm just asking, should we add something like:
# this is technically an offense, but lets make sure we don't crash
# TODO: Make this "expect_offense" when we fix regex handling.
expect_no_offenses(<<EOF) # is there an expect_no_raise ?
case node['platform']
when /debian/
%w(bmon htop).each { |p| package p }
end
end
just to make sure we're not crashing in that case too?
There was a problem hiding this comment.
I don't think we'll ever directly fix that here though so it's not really a todo. We'll transform the regexes to strings where we can and then this alert will just magically trigger later.
Detect when blocks that include more than just the package install. It's pretty common that they also include an apt_update or an include_recipe 'apt' when we're on a debian-ish platform.
I plan to open another PR to refactor the if detection with this same support. baby steps here.
Signed-off-by: Tim Smith tsmith@chef.io