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

Fix216 #217

Merged
merged 5 commits into from
May 13, 2016
Merged

Fix216 #217

merged 5 commits into from
May 13, 2016

Conversation

bastelfreak
Copy link
Member

can somebody review? This should fix #216

this addresses voxpupuli#216
getvar() comes from stdlib, scope.lookupvar is a dirty erb hack.
rubocop 0.39.0 recommended it. 0.40.0 doesnt
¯\_(ツ)_/¯
this doesn't make any sence here. We're creating a new subclass and
don't want to redeclare puppet.
@@ -265,8 +265,7 @@
# is set to for example "eth1" or "bond0.73".
if ($listenip != undef) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this whole thing could be turned into a case statement to a little clearer

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but lets leave that for another time maybe?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep. referenced in another issue.

@@ -117,7 +116,8 @@ def package_provider_for_gems
database_name: 'zabbix-server',
database_user: 'zabbix-server',
database_password: 'zabbix-server',
zabbix_server: 'localhost')
zabbix_server: 'localhost',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sigh, of course that now causes a build failure _

Copy link
Member

@rnelson0 rnelson0 May 13, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try

let :params do
  super().merge({
    # stuff
  })

Pass the hash! You can do a trailing comma this way because it's a hash, not an argument to a method.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will throw:

spec/classes/database_mysql_spec.rb:11:21: C: Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter.
        facts.merge({ ...

the rubocop config in this module is a bit stricter than our default one, and I want to keep that behavior. I will remove the commit as @jyaworski also seems to be fine with that.

@bastelfreak
Copy link
Member Author

@igalic IMO I should revert the last commit, then we can merge it. Opinions?

@jyaworski
Copy link
Member

Sounds good to me.

@rnelson0 rnelson0 merged commit f58bdb0 into voxpupuli:master May 13, 2016
@bastelfreak bastelfreak deleted the fix216 branch May 13, 2016 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

undefined local variable or method `int_name'
4 participants