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

Fix #820 #846

Merged
merged 1 commit into from
Jan 4, 2018
Merged

Fix #820 #846

merged 1 commit into from
Jan 4, 2018

Conversation

alvagante
Copy link
Collaborator

@alvagante alvagante commented Nov 21, 2017

Should fix #820
Have had to add a new parameter to client_config_register.

Description

Related Issue

Fixes #820 .

Motivation and Context

How Has This Been Tested?

General

  • Update README.md with any necessary configuration snippets

  • New parameters are documented

  • New parameters have tests

  • Tests pass - bundle exec rake validate lint spec

@alvagante alvagante changed the title WIP: Fix 820 WIP: Fix #820 Nov 21, 2017
@@ -13,7 +13,9 @@

include ::sensu

file { "${::sensu::conf_dir}/subscription_${name}.json":
$sanitized_name=regsubst($name, '[^0-9A-Za-z.-]', '_', 'G')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please comment on what this function is doing?

Copy link
Collaborator

Choose a reason for hiding this comment

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

regex explained in words can be really useful so you dont have to dissect what it is doing

it { should contain_file('/etc/sensu/conf.d/subscription_roundrobin_foo.json').with(:ensure => 'present' ) }
end

context 'with char : in title in windows' do
Copy link
Collaborator

Choose a reason for hiding this comment

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

Awesome! Love seeing those tests :)

@@ -34,6 +34,11 @@ def initialize(*args)
defaultto '/etc/sensu/conf.d/'
end

newparam(:file_name) do
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be documented in the README

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually in README we document only sensu::subscription and from here we call sensu_client_subscription using the new file_name param (f8b25fe#diff-dbd03c1a865315be6893da851b471baeR29) but not exposing it as param for sensu::subscription.
So either we expose file_name parameter also in sensu::subscription and we document it, or we just leave it a an internal setting that users don't have to mess it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah ok, it can stay internal

Fix for sensu#820

FIxed sensu_client_subscription type

Added regexp explanation and vagrant test
@alvagante alvagante changed the title WIP: Fix #820 Fix #820 Dec 4, 2017
@ghoneycutt ghoneycutt merged commit 94b0f03 into sensu:master Jan 4, 2018
@ghoneycutt
Copy link
Collaborator

Released in v2.43.0

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.

Roundrobin subscriptions on Windows aren't configured
2 participants