-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Enhance Kapacitor module and state. #46294
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
Conversation
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.
Hi @aabouzaid - I have a couple of requests and one question on this change. Thanks!
salt/modules/kapacitor.py
Outdated
@@ -105,6 +121,7 @@ def _run_cmd(cmd): | |||
def define_task(name, | |||
tick_script, | |||
task_type='stream', | |||
dbrps=None, |
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 you move this new kwarg to the end of the list? We don't want to change the API unnecessarily here.
salt/states/kapacitor.py
Outdated
@@ -32,6 +34,7 @@ def __virtual__(): | |||
def task_present(name, | |||
tick_script, | |||
task_type='stream', | |||
dbrps=None, |
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.
Same comment here as above.
A list of databases and retention policies in "dbname"."rpname" format | ||
to fetch data from. For backward compatibility, the value of | ||
'database' and 'retention_policy' will be merged as part of dbrps. | ||
|
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 you add a .. versionadded:: Fluorine
tag to these docs (and do the same for the docs in the module)?
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.
Sure, but how I can add it for mixed sections like this:
'''
Kapacitor state module.
:configuration: This module accepts connection configuration details either as
parameters or as configuration settings in /etc/salt/minion on the relevant
minions::
kapacitor.unsafe_ssl: 'false'
kapacitor.protocol: 'http'
kapacitor.host: 'localhost'
kapacitor.port: 9092
This data can also be passed into pillar. Options passed into opts will
overwrite options passed into pillar.
.. versionadded:: 2016.11.0
'''
Since kapacitor.unsafe_ssl
and kapacitor.protocol
are new, but rest are old (2016.11.0
)
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.
Good question. I think you can put that in the configuration block, indented. But I am not sure on what the formatting will look like. It might just be prudent to add a comment in that configuration block stating that those options are new to Fluorine.
salt/states/kapacitor.py
Outdated
@@ -63,6 +71,7 @@ def task_present(name, | |||
|
|||
task = __salt__['kapacitor.get_task'](name) | |||
old_script = task['script'] if task else '' | |||
task_dbrps = [{'db': dbrp[0], 'rp': dbrp[1]} for dbrp in (dbrp.split('.') for dbrp in dbrps)] |
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.
How does this change things for people specifying database
and retention_policy
and not dbrps
? It seems like this would change the default behavior.
'database' and 'retention_policy' will be merged as part of dbrps. | ||
|
||
.. versionadded:: Fluorine | ||
|
||
database | ||
Which database to fetch data from. Defaults to None, which will use the |
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.
database
Which database to fetch data from. Defaults to None, which will use the
default database in InfluxDB.
@rallytime btw, I believe this (allow empty database) has never been "really" true, it has been changed on 28.10.2015 which means that happened even before the major change in v0.13.
Therefore, I will change this too.
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.
Ok sounds good.
What does this PR do?
Current Kapacitor module and state are missing some functionality.
This PR has following:
kapacitord
doesn't listen tolocalhost
).https
and usehttp
by default.-skipVerify
option to allow "Unsafe SSL".Tests written?
Yes
Commits signed with GPG?
Yes