Skip to content

(MODULES-4322) pre-loc edit on stdlib README #747

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

Merged
merged 1 commit into from
Apr 4, 2017
Merged

(MODULES-4322) pre-loc edit on stdlib README #747

merged 1 commit into from
Apr 4, 2017

Conversation

jbondpdx
Copy link
Contributor

Hello good module people!

Please carefully review my changes here. I've noted all questions or places I'm in doubt about with TODO, so you can search for that to find those spots easily.

Copy link
Contributor

@DavidS DavidS left a comment

Choose a reason for hiding this comment

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

added a few comments on changed sections, especially with an eye towards module authors. This was not a in-depth review.

README.markdown Outdated
* Providers

> *Note:* As of version 3.7, Puppet Enterprise no longer includes the stdlib module. If you're running Puppet Enterprise, you should install the most recent release of stdlib for compatibility with Puppet modules.

## Setup

Installing the stdlib module adds the functions, facts, and resources of this standard library to Puppet.
[Install](https://docs.puppet.com/puppet/latest/modules_installing.html) the stdlib module to add the functions, facts, and resources of this standard library to Puppet.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a sentence/link about depending on stdlib in metadata.json for module authors?

README.markdown Outdated

Acceptable input examples:

```puppet
Copy link
Contributor

Choose a reason for hiding this comment

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

these values are not puppet code - need no special highlighting

Deletes all instances of a given element from an array or hash that match a provided regular expression. A string will be treated as a one-item array.
Deletes all instances of a given element from an array or hash that match a provided regular expression. A string is treated as a one-item array.

*Note:* This function is an implementation of a Ruby class and might not be UTF8 compatible. To ensure compatibility, use this function with Ruby 2.4.0 or greater.
Copy link
Contributor

Choose a reason for hiding this comment

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

This function relies on Ruby's internal implementation, which only supports ASCII. Ruby is expected to support UTF-8 with version 2.4.0.

Might need an explanation when puppet will ship the newer ruby version.

Will need to be fixed in all instances of this note.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we know when Puppet will ship the newer Ruby version, I can add that in.

README.markdown Outdated

*Type*: rvalue.

#### `dig`

> DEPRECATED: This function has been replaced in Puppet 4.5.0. Use `dig44()` for backwards compatibility or use the new version.
Copy link
Contributor

Choose a reason for hiding this comment

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

link to puppet's own dig function, link to dig44 below.

README.markdown Outdated

*Type*: rvalue.

#### `fqdn_uuid`

Returns a rfc4122 valid version 5 UUID based on an FQDN string under the DNS namespace
Returns a rfc4122 valid version 5 UUID based on an FQDN string under the DNS namespace:
Copy link
Contributor

Choose a reason for hiding this comment

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

rfc should be upper case: RFC 4122; link to https://tools.ietf.org/html/rfc4122

please also check whether there are other RFC references.

README.markdown Outdated
Returns the lowest value of all arguments. Requires at least one argument. *Type*: rvalue.
Returns the lowest value of all arguments. Requires at least one argument.

TODO: again, what kind of argument?
Copy link
Contributor

Choose a reason for hiding this comment

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

Numbers or strings containing base 10 encoded numbers with a optional sign, and optional decimal point. See the horror that is https://github.com/puppetlabs/puppetlabs-stdlib/blob/master/lib/puppet/parser/functions/min.rb#L13-L17

README.markdown Outdated

* Run `catalog` to see the contents currently compiling catalog.
* Run `cd catalog` and `ls` to see catalog methods and instance variables.
* Run `@resource_table` to see the current catalog resource table.

#### `assert_private`
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not in alphabetic order here.

README.markdown Outdated

#### `assert_private`

Sets the current class or definition as private. Calling the class or definition from outside the current module will fail.
Sets the current class or definition as private. Calling the class or definition from outside the current module fails.
Copy link
Contributor

Choose a reason for hiding this comment

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

"defined type" instead of "definition"

* `%%`: Literal '%' character
Returns formatted time.

For example, `strftime("%s")` returns the time since Unix epoch, and `strftime("%Y-%m-%d")` returns the date.
Copy link
Contributor

Choose a reason for hiding this comment

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

link to upstream documentation for full details: https://ruby-doc.org/core-2.1.9/Time.html#method-i-strftime

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a link a couple lines down, with the arguments for this function.

DEPRECATED: replaced by `dig()`.
#### `try_get_value`

**DEPRECATED:** replaced by `dig()`.
Copy link
Contributor

Choose a reason for hiding this comment

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

lolsob

@jbondpdx
Copy link
Contributor Author

I've updated the PR based on @DavidS 's comments; will squash commits before the real PR.

README.markdown Outdated
* `encoding`
Specifies the correct file encoding. TODO: verify information, default value?
Copy link
Contributor

Choose a reason for hiding this comment

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


Default value is 'UTF-8'.

README.markdown Outdated

#### `puppet_server`

TODO: What does this return? It was unclear to me.
Copy link
Contributor

Choose a reason for hiding this comment

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

The changelog describes it as: "Addition of puppet_server fact to return agents server."

Copy link
Contributor

Choose a reason for hiding this comment

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

Returns the value of the puppet agent's server value. This is the puppet setting defining the hostname of the puppet master with which the agent should communicate.

README.markdown Outdated

Determines the root home directory.

TODO: The comments say: "This varies on PE supported platforms and may be reconfigured by the end user." How would they reconfigure it? How does it vary? What information will the user want here? (thank you!)
Copy link
Contributor

Choose a reason for hiding this comment

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

The following is what I can tell from this spec test: https://github.com/puppetlabs/puppetlabs-stdlib/blob/dce8d7b194e3f6d9bb24e076c6e835aeb671099a/spec/unit/facter/root_home_spec.rb

What is expected from each os/ how they vary:
Solaris: "/"
Linux: "/root"
Windows: nil
Macosx: "/var/root"
aix: "/root"

Because it is a facter fact, we can override (reconfigure) this like so: https://www.puppetcookbook.com/posts/override-a-facter-fact.html
I'm not sure if/ how much you'd want to include about this information as I would assume that anyone using Facter facts would know that they can be overridden.

Copy link
Contributor

Choose a reason for hiding this comment

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

Solaris 8-10 is /, and Solaris 11 is /root

I think the POINT of this fact is that it varies by platforms which is why people use a fact to detect what it is. I don't think documenting anything about PE is needed. I think the basic description is adequate for a sysadmin to know what it's for. If you want to make any modifications, maybe "Determines the root home directory. Generally this is '/root'"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds legit to me, thank you!

README.markdown Outdated
Returns the absolute value of a number; for example, '-34.56' becomes '34.56'. Takes a single integer and float value as an argument. *Type*: rvalue.
Returns the absolute value of a number. For example, '-34.56' becomes '34.56'.

Argument: A single integer and a float value as an argument.
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be misleading - checked the function and its in there too as a comment which we should change. But it should be more: Accepts a single argument of either an integer or float value.

Copy link
Contributor

Choose a reason for hiding this comment

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

We need a standard way of documenting arguments, and all functions should document their arguments.

Arguments: abs(Variant[Integer, Float] $arg)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, there's some effort in here to standardize some of that, but it might be that we do more of that as we transition stuff to Puppet 4.

README.markdown Outdated
Converts the case of a string or all strings in an array to camel case. *Type*: rvalue.
Converts the case of a string or all strings in an array to mixed case (both uppercase and lowercase).

Arguments: TODO how do I use this function?
Copy link
Contributor

Choose a reason for hiding this comment

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

Since camel case is (as far as I know) a technical term I would leave it in here?
Requires inputs of either array or string. Returns the same type of argument as it received but in CamelCase form.

Copy link
Contributor

Choose a reason for hiding this comment

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

arguments: camelcase(Variant[String, Array[String]] $arg)

README.markdown Outdated
Deletes a determined indexed value from an array. For example, `delete_at(['a','b','c'], 1)` returns ['a','c']. *Type*: rvalue.
Deletes a determined indexed value from an array.

For example: `delete_at(['a','b','c'], 1)` returns ['a','c']. *Type*: rvalue.
Copy link
Contributor

Choose a reason for hiding this comment

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

Missed the newline break for the type.

README.markdown Outdated

*Type*: statement.

#### `ensure_resources`

Takes a resource type, title (only hash), and a hash of attributes that describe the resource(s).
TODO: I'm not sure I understand this one exactly. Need to clarify and rewrite.
Copy link
Contributor

@HelenCampbell HelenCampbell Mar 28, 2017

Choose a reason for hiding this comment

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

I'll take a bash describing here, forgive me if it doesn’t help!
So if I want to ensure a resource is exists I can use this function. I pass in the type of the resource I'm looking for, it's title and a hash of attributes that will be checked against the resource, and if the resource isn’t present it will attempt creating it.
So with the example:

ensure_resource('user', 'jean', {'ensure' => 'present'})

I am trying to ensure there is a user resource called jean, and I want to make sure it has attributes 'ensure' => 'present'.
If the resource does not exist it will create it:

user { 'jean':
	ensure => present,
}

If the (jean) resource already exists but does not match the specified parameters:

user { 'jean':
	ensure => absent,
}

this function will attempt to recreate the resource leading to a duplicate
resource definition error. Rough example: Error: Duplicate declaration: User[jean] is already declared in file /...

In the other example it uses a hash of titles - this simply loops through the titles performs the aforementioned functionality on each of them.

Copy link
Contributor

Choose a reason for hiding this comment

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

(I updated helen's comment for code coloring)

I think Helen's comment was for ensure_resource() so I'll comment for ensure_resource(), ensure_resources(), ensure_packages(), and create_resources() as they are all related.

  • create_resources takes a hash (almost always being read in from Hiera) and turns it into resource declarations. This is in core puppet.
  • ensure_resources is create_resources but won't conflict with already-declared resources.
  • ensure_packages is ensure_resources but hardcoded to be only for package resources (the most common use-case for not wanting to conflict with already-declared resources; it predates ensure_resources)
  • ensure_resource is ensure_resources but for a single resource only.

README.markdown Outdated

*Type*: rvalue.

#### `has_ip_address`

Returns 'true' if the client has the requested IP address on some interface. This function iterates through the `interfaces` fact and checks the `ipaddress_IFACE` facts, performing a simple string comparison. *Type*: rvalue.
Returns `true` if the client has the requested IP address on some interface.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would keep the second sentence here as it's important to know where it's checking against and how. Otherwise you'd have to look at the function code itself to find out.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

README.markdown Outdated

#### `has_ip_network`

Returns 'true' if the client has an IP address within the requested network. This function iterates through the `interfaces` fact and checks the `network_IFACE` facts, performing a simple string comparision. *Type*: rvalue.
Returns `true` if the client has an IP address within the requested network.
Copy link
Contributor

Choose a reason for hiding this comment

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

Again would leave in the second sentence for clarity to what it's being checked against.

README.markdown Outdated
#### `type3x`
*Type*: rvalue.

#### `type3x` TODO: is this deprecated at this point?
Copy link
Contributor

Choose a reason for hiding this comment

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

It claims to be deprecated in the docs however there are no warnings which would make me worry about removing it. Most likely it will stay in and we will add a deprecation warning, then in the release after it will be fully removed.
Arguments can be any of the following:

  • string
  • array
  • hash
  • float
  • integer
  • boolean

Copy link
Contributor

Choose a reason for hiding this comment

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

It was deprecated when it was added, and documented as such:

It can be removed when we drop puppet 3, as stated. It doesn't have to print a warning to be deprecated.

README.markdown Outdated

Values: String.

Default value: `undef`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably doesn't make sense to have a default for a required parameter.

README.markdown Outdated

Values: `true`, `false`.

Default value: `undef`.
Copy link
Contributor

Choose a reason for hiding this comment

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

This effectively defaults to false, and since undef isn't a valid value we should probably say that false is the default.

README.markdown Outdated

Values: String.

Default value: `undef`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Defaults to the title's value.

README.markdown Outdated

#### `pe_minor_version`

Returns the minor version of Puppet Enterprise that is installed.
Copy link
Contributor

Choose a reason for hiding this comment

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

The 4 facts is_pe pe_version pe_major_version and pe_minor_version don't apply to puppet-agent installs as it is not PE-specific, so they do not report anything on platforms newer than PE 3.x. I propose removing these facts when PE 3 compatibility is dropped.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or pe_patch_version

README.markdown Outdated

#### `puppet_server`

TODO: What does this return? It was unclear to me.
Copy link
Contributor

Choose a reason for hiding this comment

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

Returns the value of the puppet agent's server value. This is the puppet setting defining the hostname of the puppet master with which the agent should communicate.

README.markdown Outdated
#### `type3x`
*Type*: rvalue.

#### `type3x` TODO: is this deprecated at this point?
Copy link
Contributor

Choose a reason for hiding this comment

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

It was deprecated when it was added, and documented as such:

It can be removed when we drop puppet 3, as stated. It doesn't have to print a warning to be deprecated.

README.markdown Outdated

Returns a string description of the type when passed a value. Type can be a string, array, hash, float, integer, or boolean. This function will be removed when Puppet 3 support is dropped and the new type system can be used. *Type*: rvalue.
Arguments: TODO: what is the argument to use?
Copy link
Contributor

Choose a reason for hiding this comment

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

arguments: type3x(Any $arg)

* Plus all of the above, but with non-integer items in arrays or maximum / minimum argument.

*Type*: statement.
Validates an integer or an array of integers. Terminates catalog compilation if any of the checks fail.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just remove all documentation except the line stating that it is deprecated? We don't need to explain how to use deprecated functionality.

README.markdown Outdated

*Note:* validate_string(undef) will not fail in this version of the functions API (incl. current and future parser).
*Note:* validate_string(`undef`) will not fail in this version of the functions API (including current and future parser). TODO: Is this true for Puppet 3 only?
Copy link
Contributor

Choose a reason for hiding this comment

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

undef also validates as a string on my puppet 4.9, so I'd say on both 3 and 4.

README.markdown Outdated
Arguments:

* An integer or an array of integers, as the first argument.
* Optionally, a maximum, as the second argument. All elements of the first argument must be equal to or less than this maximum. [TODO: Does (All elements of) mean the sum of all elements has to be equal to or less than the max, or that each element has to be equal to or less than the max. I can fix the instances after this too, don't worry about tagging them.]
Copy link
Contributor

Choose a reason for hiding this comment

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

"All elements of the first argument..." means "If the first argument is an array, all values in the array..."

Copy link
Contributor

Choose a reason for hiding this comment

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

Arrays in computer science are based on set theory, and "element" is a set theory word for "object in a set."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, that helps me, thank you.

@jbondpdx
Copy link
Contributor Author

jbondpdx commented Apr 4, 2017

@HelenCampbell and @hunner , thank you!

@jbondpdx
Copy link
Contributor Author

jbondpdx commented Apr 4, 2017

Requested changes are made, except for my one outstanding question for @hunner above. If that code sample looks ok, this can be merged; if I need to fix it, I can do that. Oh, I can squash these too, if you want me to.

@jbondpdx
Copy link
Contributor Author

jbondpdx commented Apr 4, 2017

OK, this is squashed and updated, so maybe ready for merge now?

@jbondpdx jbondpdx changed the title FOR REVIEW: (MODULES-4322) 1st edit pass on stdlib README (MODULES-4322) pre-loc edit on stdlib README Apr 4, 2017
@hunner hunner merged commit bf4ee15 into puppetlabs:master Apr 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants