-
Notifications
You must be signed in to change notification settings - Fork 54
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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