Skip to content
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

Fixes foodcritic, previous fix caused all checks to be ignored #86

Merged
merged 1 commit into from
Nov 26, 2016

Conversation

madeddie
Copy link
Contributor

The .foodcritic in its current form causes foodcritic not to check a single rule.

If you don't like these fixes, I can create a .foodcritic that skips these specific rules.

@legal90
Copy link
Contributor

legal90 commented Nov 24, 2016

@johnbellone Method package in vault_installation_package looks weird. Is it a built-in Chef resource or some poise wrapper? If first, then maybe we should just remove checksum attribute and replace action :uninstall with action :remove?

@madeddie
Copy link
Contributor Author

If it's a wrapper, it might be best to rename it to some custom name, so foodcritic doesn't get worried.

@johnbellone
Copy link
Contributor

That method should be the built-in package resource, and it should choose the appropriate provider based on the platform. In newer cookbooks, because of Ruby's shadowing, we've modified what we call these variables. In following the naming convention there it would be system_package_checksum and that would come from options[:package_checksum].

@legal90
Copy link
Contributor

legal90 commented Nov 25, 2016

Ah, thanks, I got it. Actually, I thought that action :uninstall and attribute checksum are not supported for package resource at all, because they are not mentioned in the Chef documentation.

But it seems like they are supported for windows:
https://github.com/chef/chef/blob/1991cf97660a38e704eb4cd9609ed6ea84a35442/lib/chef/resource/windows_package.rb#L51
https://github.com/chef/chef/blob/1991cf97660a38e704eb4cd9609ed6ea84a35442/lib/chef/resource/chocolatey_package.rb#L27

So, about the PR - it looks good to me

@johnbellone johnbellone merged commit cf1d963 into sous-chefs:master Nov 26, 2016
@legal90
Copy link
Contributor

legal90 commented Nov 26, 2016

@madeddie Thanks for the contribution! 👍

@madeddie madeddie deleted the fix_foodcritic branch November 26, 2016 11:41
@lock
Copy link

lock bot commented May 19, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators May 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants