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

Enable template options #61

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Enable template options #61

wants to merge 18 commits into from

Conversation

tomas-edwardsson
Copy link
Contributor

What it does

Enables template options via --opt VAR=SOMETHING

What it does NOT

Break how everything used to work, so old templates still work

Description

$ okconfig addtemplate www.okconfig.org --template http \
        --opt VIRTUAL_HOST=vhost.okconfig.org \
        --opt SEARCH_STRING=something
Saved /etc/nagios/okconfig/hosts/default/www.okconfig.org-http-vhost.okconfig.org.cfg

Contents of the file (/etc/nagios/okconfig/hosts/default/www.okconfig.org-http-vhost.okconfig.org.cfg) become

define service {
    use         okc-check_http
    host_name       www.okconfig.org
    contact_groups      default
    service_description HTTP vhost.okconfig.org
    #check_command      okc-check_http

    __VIRTUAL_HOST      vhost.okconfig.org
    #__URI          /
    __SEARCH_STRING something
    #__RESPONSE_WARNING 2.0
    #__RESPONSE_CRITICAL    10.0
    #__PORT         80
    #__ON_REDIRECT           follow

}

Then there is the configuration for the template options.
/usr/share/okconfig/examples/http.yml

filename:   "{HOSTNAME}-http-{VIRTUAL_HOST}.cfg"
parms:
    VIRTUAL_HOST: 
        default:    k:HOSTNAME
        summary:    Virtual host to query for
    URI:
        default:    /
    SEARCH_STRING:
        default:    
    RESPONSE_WARNING:
        default:    2.0
        summary:    Response time in seconds till warning
    RESPONSE_CRITICAL:
        default:    10.0
        summary:    Response time in seconds till critical
    PORT:
        default:    80
    ON_REDIRECT:
        default:    follow
        summary:    Action to on redirects, eg <ok|warning|critcal|follow|sticky|stickyport>

It still uses the example file which now looks like this:

define service {
    use         okc-check_http
    host_name       HOSTNAME
    contact_groups      GROUP
    service_description HTTP ^VIRTUAL_HOST
    #check_command      okc-check_http

    __VIRTUAL_HOST      ^VIRTUAL_HOST
    #__URI          ^URI
    #__SEARCH_STRING    ^SEARCH_STRING
    #__RESPONSE_WARNING ^RESPONSE_WARNING
    #__RESPONSE_CRITICAL    ^RESPONSE_CRITICAL
    #__PORT         ^PORT
    #__ON_REDIRECT           follow

}

@coveralls
Copy link

Coverage Status

Coverage increased (+2.6%) when pulling 1b1f3b3 on templateopts into dff24b1 on master.

@palli
Copy link
Contributor

palli commented Jun 28, 2014

OK, starting to look at this now. Dumping questions as i see them.

Is the feature not more appropriately named template variables ?

return template_output, filename

def _parse_template_opts(opt_file):
import yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move this import to the top of the file.

Also, please put yaml dependency in requirements.txt, rpm spec file and debian/ directory

@palli
Copy link
Contributor

palli commented Jul 2, 2014

Ok i didnt run this yet, but the code diff looks good.

Then i just stumbled on to an idea that might seem idiodic but...

Seems like in your http example there is 1:1 mapping between attributes, and the "template option names".

Why not have this --opt foo=bar describe any attribute name ? Benefits:

  • We don't have to remove all default values from the example files (maintains cleanliness)
  • We support changing any attribute, not just the options that we provide
  • We don't need the extra yaml file

Cons:

  • We haven't solved the "cannot add the same template twice" problem, but there are other ways to work around that (like providing --destination-filename paramater, or not overwrite the file by default)

Again, we come back to we need more concrete examples. Currently the http feature is kind of already implemented in adagios via okconfig > add service

@palli
Copy link
Contributor

palli commented Jul 2, 2014

(so a quick summary, i dont see a problem with the code per se, but i worry that the feature could be implemented without a need for another config file.

@tomas-edwardsson
Copy link
Contributor Author

The destination-filename is a stumbling block, plus alternate service_descriptions which I want to address. Instead of going with the mssql, I'm going to post an example using check_postgres which highlights the problem more adequetaly. There we have server based checks and then we have per-database checks. The per database checks would need to be noted in the service_description and I would see adding a postgres-{hostname}-{database}.cfg for each database.

Why a configuration file? I want to enrich the user experience of arguments where there is a description of what the option is and even going with enum fields where there are only a few valid options. No functionality has been implemented yet but consumers can read the description field and I'm working on the functionality of okconfig addtemplate -i <template_name> which will invoke a interactive mode and allow you via command line to set template variables.

The 1:1 mapping is almost correct but I'm offering for sane defaults where something like VIRTUAL_HOST can be derived from HOSTNAME by default but the user is allowed to specify his own VIRTUAL_HOST where appropriate.

@palli
Copy link
Contributor

palli commented Jul 2, 2014

Ok sounds good. Postgres template sounds nice.

@palli
Copy link
Contributor

palli commented Jul 3, 2014

Additionally, thoughts on a less confusing name ? (suggestion: template variables)

@pall-valmundsson
Copy link
Contributor

I have a few templates that would benefit from this. Basically all checks that belong on a host that have to check via some 3rd party, e.g. iLO checks need an IP address parameter and vSphere node checks need various parameters for querying the vSphere API.

I like the "template variables" name better as well.

@palli
Copy link
Contributor

palli commented Nov 16, 2014

Are you still working on this ?

@@ -44,6 +44,7 @@
examples_directory_local= config.examples_directory_local
destination_directory = config.destination_directory
import socket
import yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like yaml is a new dependency. Should be marked as such in packaging.

@sonarcloud
Copy link

sonarcloud bot commented Nov 26, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 13 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

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.

4 participants