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

(MODULES-4225) add strings to ntp module #366

Merged
merged 2 commits into from
Jan 11, 2017
Merged

(MODULES-4225) add strings to ntp module #366

merged 2 commits into from
Jan 11, 2017

Conversation

jbondpdx
Copy link
Contributor

Added strings to init.pp

Made a few very minor changes to the README.

# Main class, includes all other classes.
#
# @example Declaring the class
# include example
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this may have been copy pasted over by mistake?

# @param interfaces_ignore [Array[String]] Specifies one or more ignore pattern for the NTP listener configuration (for example: all, wildcard, ipv6). Default value: [ ].
# @param keys [Array[String]] Distributes keys to keys file. Default value: [ ].
# @param keys_controlkey [Optional. Ntp::Key_id] Specifies the key identifier to use with the ntpq utility. Value in the range of 1 to 65,534 inclusive. Default value: ' '.
# @ param keys_enable [Boolean] Whether to enable key-based authentication. Default value: false.
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo here, because of the space it's not picking up the comment

# @param tos_ceiling [Optional Integer[1]] Specifies the ceiling tos option. Default value: 15.
# @param tos_cohort [Variant Boolean, Integer[0,1]] Specifies the cohort tos option. Valid options: 0 or 1. Default value: 0.
# @param tinker [Boolean] Whether to enable tinker options. Default value: false.
# @parudlc_stratum [Optional Integer[1,15]]. Specifies the stratum the server should operate at when using the undisciplined local clock as the time source. This value should be set to no less than 10 if ntpd might be accessible outside your immediate, controlled network. Default value: 10.am udlc [Boolean] Specifies whether to configure NTP to use the undisciplined local clock as a time source. Default value: false.
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo here too it looks like

@HAIL9000
Copy link
Contributor

So when I ran this through puppet strings everything looked great except that four parameters are not documented:

  • keys_enable (Boolean)
  • udlc (Boolean)
  • udlc_stratum (Optional[Integer[1,15]])
  • authprov (Optional[String])

keys_enable and udlc_stratum are captured by the typos noted above. udlc maybe just got missed? I see a comment for it in the README. But authprov I don't even see in the README... is that just something we don't document?

@jbondpdx
Copy link
Contributor Author

Yeah, I probably missed udlc... authprov, I don't know about. I suspect it's a victim of our manual readme process. What does it do? I can add it.

@HAIL9000
Copy link
Contributor

So looks like this originally came from #306, according to the pull request:

This parameter is used in some versions of NTPd (such as Novell DSfW) to
enable compatibility with W32Time.

@jbondpdx
Copy link
Contributor Author

OK, @HAIL9000 , PR is updated!

@HAIL9000
Copy link
Contributor

Since the ntp class is the only class users should care about, it's the only one that needs any real documentation. However, strings will detect and make HTML pages for all classes no matter what and will issue warnings if they are not documented:

[warn]: Missing documentation for Puppet class 'ntp::config' at manifests/config.pp:2.
[warn]: Missing documentation for Puppet class 'ntp::install' at manifests/install.pp:2.
[warn]: Missing documentation for Puppet class 'ntp::service' at manifests/service.pp:2.

So it seems like it might be prudent just to add a comment to each saying the classes function (as described in the README), and that it is API private and so the user should not be creating instances of it.

@HAIL9000
Copy link
Contributor

So the @api tag is not currently supported for classes (it is for functions though). I filed a ticket to include it for classes (PDOC-156), but we could potentially include it in the meantime in addition to the comments I suggested above. That way, when class support is added it will Just Work.

The nice thing about the @api tag is that when it does work it adds this clear warning to users on the generated HTML page:

screen shot 2017-01-11 at 12 26 02 pm

@jbondpdx
Copy link
Contributor Author

@HAIL9000 this is updated again

@@ -1,4 +1,6 @@
# Private Class
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is redundant now I think we can scrap it

@@ -1,3 +1,5 @@
# @api private
# This class handles the ntp service. Avoid modifying private classes.
#
Copy link
Contributor

Choose a reason for hiding this comment

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

Absurdly small nitpick, I think we should remove this blank comment line so our formatting is consistent across all the classes

@HAIL9000 HAIL9000 merged commit 4ab7081 into puppetlabs:master Jan 11, 2017
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.

2 participants