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

(GH-578) etc_dir should be configurable #741

Merged
merged 1 commit into from
Jul 25, 2017

Conversation

Phil-Friderici
Copy link
Collaborator

No description provided.

@@ -26,5 +26,6 @@
:ipaddress => '127.0.0.1',
:kernel => 'Linux',
:osfamily => 'RedHat',
:fqdn => 'testfqdn',
Copy link
Collaborator

Choose a reason for hiding this comment

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

your fqdn isnt fully qualified, perhaps testfqdn.example.com

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@Phil-Friderici
Copy link
Collaborator Author

Phil-Friderici commented Jul 13, 2017

Wow, this small parameter does touch a lot of resources.

The sensu::check class does not source in $etc_dir from sensu main class. It does use it's own (same) default values. Should I change sensu::check to also source values from main class ?

@Phil-Friderici
Copy link
Collaborator Author

will rebase

@Phil-Friderici
Copy link
Collaborator Author

rebased & ready for review

@Phil-Friderici Phil-Friderici changed the title (GH-578) etc_dir should be configurable (WIP) (GH-578) etc_dir should be configurable Jul 14, 2017
# String. Absolute path to the sensu etc directory.
# Default: '/etc/sensu' on RedHat and Debian
# 'C:/opt/sensu' on windows
# undef on all other platforms
Copy link
Collaborator

Choose a reason for hiding this comment

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

what happens if this is undef? Maybe the default should be /etc/sensu ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That would make sense in most cases I guess.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed the behaviour to default to '/etc/sensu'

@Phil-Friderici
Copy link
Collaborator Author

Phil-Friderici commented Jul 17, 2017

@tuxmea Let's add support for new OSfamilies in specific PRs/commits. Guess it needs more work than this.

@@ -30,6 +136,106 @@
}
end

# FIXME: The following resource checks are only testing $sensu_etc_dir specific values
Copy link
Collaborator

Choose a reason for hiding this comment

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

what else should they test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These tests only checks for the $sensu_etc_dir specific details. They don't test the other file resource details that should be tested as well. Eg:

      it do
        should contain_file('C:/opt/sensu/bin/sensu-client.xml').with({
          'ensure'  => 'file',
          'content' => content,
          ... and other parameters....
        })
      end

There is still a lot of test to add for all the managed resources and functionality. But this is out of scope for this PR.

@Phil-Friderici
Copy link
Collaborator Author

will rebase

@Phil-Friderici
Copy link
Collaborator Author

rebased

@ghoneycutt
Copy link
Collaborator

Sorry to ask you to rebase again, though we've implemented data types and all PR's will need to be rebased.

@Phil-Friderici
Copy link
Collaborator Author

et voilà, rebased for a good thing ;)

@ghoneycutt ghoneycutt merged commit 2c64888 into sensu:master Jul 25, 2017
@ghoneycutt
Copy link
Collaborator

Solves #578

@ghoneycutt
Copy link
Collaborator

Thank you! Released in v2.28.0

@Phil-Friderici Phil-Friderici deleted the GH-578 branch July 26, 2017 08:24
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.

3 participants