Make http_header repeatable (fixes #8573) #8574
Conversation
itl/command-plugins.conf
Outdated
| @@ -502,7 +502,8 @@ object CheckCommand "http" { | |||
| } | |||
| "-k" = { | |||
| value = "$http_header$" | |||
| description = "Any other tags to be sent in http header" | |||
| description = "Any other tags to be sent in http header (may be repeated)" | |||
| repeat_key = true | |||
There was a problem hiding this comment.
Are you sure this is needed?
"If the argument value is an array, repeat the argument key, or not. Defaults to true (repeat)."
– https://icinga.com/docs/icinga-2/latest/doc/09-object-types/#checkcommand-arguments
There was a problem hiding this comment.
Argh ... probably not. But it is specified explicitly in several other places (even in the same config file).
It also might have pretty strange results with the current default when someone repeats a key for a value where the plugin does not allow repetition. But that's a different story.
There was a problem hiding this comment.
So... we'll probably close this one?
There was a problem hiding this comment.
The question is whether to close it or add the flag for clarification's sake. It doesn't hurt after all, and IMO it makes the usage of the command clearer - I wouldn't have brought this up at all had the repeat_key attribute been specified for the command (or had it not been specified for other command options).
There was a problem hiding this comment.
Yes, it makes sense to clarify this in the descriptions. Please have a look how the other commands describe it.
|
There is unfortunately not too much of standard practice in wording the description it seems. I adjusted the wording in the documentation to the most common statement. |
There was a problem hiding this comment.
Rebase the PR onto the current master, so that it contains only your changes. Sure, I've build PRs on each other myself, but 413 commits suggest that something went terribly wrong.
Or – better:
- Pull our master
- Checkout your branch
git merge --no-edit mastergit reset --soft master
This should make you fall back to I've changed and git-added everything, but not committed, yet. Of course on top of the current master and with everything of your desired changes.
|
Indeed, something did go terribly wrong ... should be OK now. |
|
... except |
doc/10-icinga-template-library.md
Outdated
| @@ -656,7 +656,7 @@ http_method | **Optional.** Set http method (for example: H | |||
| http_maxage | **Optional.** Warn if document is more than seconds old. | |||
| http_contenttype | **Optional.** Specify Content-Type header when POSTing. | |||
| http_useragent | **Optional.** String to be sent in http header as User Agent. | |||
| http_header | **Optional.** Any other tags to be sent in http header. | |||
| http_header | **Optional.** Any other tags to be sent in http header. Can be an array if multiple headers should be passed to `check_http'. | |||
There was a problem hiding this comment.
Why not to take a shorter clarification...
| @@ -502,7 +502,7 @@ object CheckCommand "http" { | |||
| } | |||
| "-k" = { | |||
| value = "$http_header$" | |||
| description = "Any other tags to be sent in http header" | |||
| description = "Any other tags to be sent in http header (may be repeated)" | |||
There was a problem hiding this comment.
That's how I formulated it in the first PR. I took it from your answer "Please have a look how the other commands describe it." that you weren't satisfied with it (and I agree, it could be clearer).
There was a problem hiding this comment.
So the other commands do it like you did here?
There was a problem hiding this comment.
As I wrote above, I could not make out a "standard phrasing" for the fact that a parameter accepts arrays. In the command definitions sometimes it's "may be repeated", sometimes "may be used repeatedly".
In the documentation file the wording is even more inconsistent (just a few examples):
doc/10-icinga-template-library.md:1208:smtp_command | **Optional.** SMTP command (may be used repeatedly).
doc/10-icinga-template-library.md:1209:smtp_response | **Optional.** Expected response to command (may be used repeatedly).
doc/10-icinga-template-library.md:206:apt_include | **Optional.** Include only packages matching REGEXP. Can be specified multiple times the values will be combined together.
doc/10-icinga-template-library.md:207:apt_exclude | **Optional.** Exclude packages matching REGEXP from the list of packages that would otherwise be included. Can be specified multiple times.
doc/10-icinga-template-library.md:240:by_ssh_command | **Required.** The command that should be executed. Can be an array if multiple arguments should be passed to `check_by_ssh`.
doc/10-icinga-template-library.md:356:disk\_ignore\_eregi\_path | **Optional.** Regular expression to ignore selected path/partition (case insensitive). Multiple regular expression strings must be defined as array.
doc/10-icinga-template-library.md:1937:nscp_disk_exclude | **Optional.** Drive character, default to none. Can be an array of drive characters if multiple drives should be excluded.
But generally, the documentation is a bit more verbose than the descriptions from the command definition.
|
It makes sense not to specify default options explicitly, agreed. What about the other ones? There are 9 other instances in command-plugins.conf alone ... |
fixes #8573.