Skip to content

all options documentation #36

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

Closed

Conversation

colinsurprenant
Copy link
Contributor

improve doc for all options.

@karenzone
Copy link
Contributor

@colinsurprenant Please verify the default codec (Line 3) and delete my comment (Line 5). I didn't know, so I set it to plain as a placeholder when I set up the doc.

Copy link
Contributor

@karenzone karenzone left a comment

Choose a reason for hiding this comment

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

Thanks you for clearing up my questions and adding so much good content. I'd like to see it again to be sure that everything still builds cleanly after changes. Let me know if you have questions or would like to discuss anything here.

@@ -58,21 +55,28 @@ This plugin supports the following configuration options plus the <<plugins-{typ
[cols="<,<,<",options="header",]
|=======================================================================
|Setting |Input type|Required
| <<plugins-{type}s-{plugin}-get>> |<<string,string>>|No
| <<plugins-{type}s-{plugin}-get>> |<<array,array>>|No
| <<plugins-{type}s-{plugin}-walk>> |<<array,array>>>|No
Copy link
Contributor

Choose a reason for hiding this comment

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

Extra > after <<array,array>>


[id="plugins-{type}s-{plugin}-import-mibs"]
==== Importing MIBs

This plugin includes management information bases (MIBs) for most use cases.
If you use custom MIBs, you need to import them.
This plugin alreadyincludes the IETF MIBs (management information bases) and these do not need to be imported.
Copy link
Contributor

Choose a reason for hiding this comment

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

Need a space in alreadyincludes

ToDO: Update description and values with correct info
A **single user** can be configured and will be used for all defined SNMPv3 hosts.
Multiple snmp input declarations will be needed if multiple SNMPv3 users are required.
These options are not required when not using SNMPv3.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change to "These options are required only if you are using SNMPv3" to make it easier to parse.

| <<plugins-{type}s-{plugin}-hosts>> |<<array,array>>|No
| <<plugins-{type}s-{plugin}-interval>> |<<number,number>>|No
| <<plugins-{type}s-{plugin}-mib_paths>> |<<path,path>>|No
| <<plugins-{type}s-{plugin}-oid_root_skip>> |<<boolean,boolean>>|No
| <<plugins-{type}s-{plugin}-security_name>> |<<string,string>>|No
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the way you have ==== SNMPv3 Authentication Options called out in a separate section for the detailed descriptions. It would be good to break the summary table that way as well. So we'd have the table for SNMP Input Configuration Options and then a table for SNMPv3 Authentication Options immediately following.

| <<plugins-{type}s-{plugin}-auth_pass>> |<<password,password>>|No
| <<plugins-{type}s-{plugin}-priv_protocol>> |<<string,string>>, one of `["des", "3des", "aes", "aes128", "aes192", "aes256"]`|No
| <<plugins-{type}s-{plugin}-priv_pass>> |<<password,password>>|No
| <<plugins-{type}s-{plugin}-security_level>> |<<string,string>>, one of `["noAuthNoPriv", "authNoPriv", "authPriv"]`|No
|=======================================================================
Copy link
Contributor

Choose a reason for hiding this comment

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

Options should be alphabetized in the table and in the descriptions so that they're easier to find.

The `security_level` option specifies the SNMPv3 security level between Authentication, No Privacy; Authentication, Privacy; or no Authentication, no Privacy

* Value can be any of: `noAuthNoPriv`, `authNoPriv`, `authPriv`
* There is no default value for this setting

==== More configuration examples
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the options in the config examples are for SNMP Input Configuration Options, maybe we shoud move the examples up into that section.

@jsvd
Copy link
Member

jsvd commented Oct 24, 2018

I addressed some of the comments here and merged this in c936119.
Lingering comments I didn't address:

Since the options in the config examples are for SNMP Input Configuration Options, maybe we shoud move the examples up into that section.

I like the way you have ==== SNMPv3 Authentication Options called out in a separate section for the detailed descriptions. It would be good to break the summary table that way as well. So we'd have the table for SNMP Input Configuration Options and then a table for SNMPv3 Authentication Options immediately following.

@colinsurprenant we can open a separated PR to address these changes and ship a future 1.0.1 version with them

@jsvd
Copy link
Member

jsvd commented Oct 24, 2018

/cc @karenzone

@colinsurprenant
Copy link
Contributor Author

perfect @jsvd ! thanks! will update unaddressed comments by @karenzone in a new PR. these are mainly cosmetic.

@colinsurprenant
Copy link
Contributor Author

thanks @karenzone for the review! 👍

@colinsurprenant
Copy link
Contributor Author

created #38 for the leftovers, we can close here.

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