-
-
Notifications
You must be signed in to change notification settings - Fork 499
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
More Rubocop fixes and README.md badges #590
Conversation
is_to_s is part of the puppet API. This is a false positive.
This should just leave the mulitple expectations violations. |
Woah! Awesome! |
@@ -24,22 +24,30 @@ def should_vhost | |||
|
|||
def self.all_vhosts | |||
vhosts = [] | |||
run_with_retries 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.
Isn't this equal: vhosts = run_with_retries { rabbitmqctl('-q', 'list_vhosts') }.split(%r{\n})
or vhosts = vhost_list.split(%r{\n})
end.split(%r{\n}).each do |exchange| | ||
end | ||
|
||
exchange_list.split(%r{\n}).each do |exchange| |
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.
Similarly: exchanges = exchange_list.split(%r{\n}).reject { |exchange| exchange =~ %r{^federation:} }
end.split(%r{\n}).find do |line| | ||
end | ||
|
||
plugin_list.split(%r{\n}).find do |line| |
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.
plugin_list.split(%r{\n}).include? resource[:name]
end.split(%r{\n}).find do |line| | ||
end | ||
|
||
vhost_list.split(%r{\n}).find do |line| |
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 also be .include?
Hopefully I've not messed anything up! The acceptance tests still all pass, but I've not done any other manual testing. |
@ekohl Rebased |
I'm not an expert for types and providers, but this looks good to me |
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.
This is looking great to me, and I think the code will be more readable too.
@alexjfisher some failing tests now. I'll merge on green. As far as release, do we want to ignore those multiple expectations warnings and do a modulesync? Or do we want to release first and do the modulesync later? |
We should do a modulesync first. This is our general workflow. Also releasing without modulesync is quite painful. |
@wyardley It looks like the tests should probably be updated. |
Whilst fixing the violation, some of the code is refactored. In particular, in some cases, exists? now returns an actual boolean instead of a string or nil. The tests for these providers are updated to match the new (more correct) behaviour.
We don't support puppet 3 anymore, so most of the code can just go.
False positive. `set_user_tags` is a rabbitmqctl subcommand.
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.
Other than that 1 thing 👍
else | ||
raise Puppet::Error, "Could not match line '#{resource[:name]} (true|false)' from list_users (perhaps you are running on an older version of rabbitmq that does not support admin users?)" | ||
end | ||
raise Puppet::Error, "Could not match line '#{resource[:name]} (true|false)' from list_users (perhaps you are running on an older version of ra bbitmq that does not support admin users?)" unless (usertags = get_user_tags) |
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'd do the assignment on a separate line. It's very long already. You also introduced spaces in "rabbitmq"
(:true if usertags.include?('administrator')) || :false | ||
else | ||
raise Puppet::Error, "Could not match line '#{resource[:name]} (true|false)' from list_users (perhaps you are running on an older version of rabbitmq that does not support admin users?)" | ||
if (usertags = get_user_tags) |
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.
@ekohl How about like this?
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 was thinking about:
usertags = get_user_tags
raise ... unless usertags
:true if usertags.include?('administrator') || :false
And I'd see if it could handle actual booleans.
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.
Look good?
Also remove statement about how the module is tested on puppet 3.
More Rubocop fixes and README.md badges
More Rubocop fixes and README.md badges
No description provided.