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

Lint, style, and parameter data types #476

Merged
merged 5 commits into from Apr 12, 2019
Merged

Lint, style, and parameter data types #476

merged 5 commits into from Apr 12, 2019

Conversation

natemccurdy
Copy link
Contributor

This PR fixes various style issues that I've had to fix internally and figured I should share them upstream:

  • Add parameter data types to all class parameters.
  • Use params.pp only for calculating default values, not static values.
  • Whitespace and indentation cleanup
  • Removes redundant relationships between resources.
  • Use modern facts and the facts hash where possible.
  • Use dig() instead of checking for sub-key existence in a Hash.

@bastelfreak
Copy link
Member

@natemccurdy can you please rebase?

@natemccurdy
Copy link
Contributor Author

@bastelfreak Yup! Thanks for reviewing so quickly.

Rebased and pushed 👍

@bastelfreak bastelfreak added enhancement New feature or request tests-fail and removed needs-rebase labels Apr 11, 2019
This commit adds parameter data types to the main classes and defines.

It also moves any static parameter defaults from params.pp to init.pp.
It's generally better to have static defaults in init.pp and leave
params.pp for any defaults that are calculated. This makes it easier to
see parameter default values when using the console in Puppet Enterprise
as well as reduces the amount of page flipping someone has to do when
casually browsing the code.
@bastelfreak
Copy link
Member

Thanks for the work @natemccurdy !

@bastelfreak bastelfreak merged commit 03f7f22 into voxpupuli:master Apr 12, 2019
@pahoughton-sf
Copy link

The change of consul::service port parameter type has broken my use of hiera lookup() for my ports

# service.yml

# agate port used in multiple places via lookup('agate::port')
agate::port: 4464

agate::consul_svc:
  agate:
    port: "%{lookup('agate::port')}"
    meta:
      example: value

# agate.pp

class agate( $port, $consul_svc ) {
  # ...
  create_resources('consul::service', $consul_svc)

What are your thoughts on reverting port back to a string value?

@natemccurdy natemccurdy deleted the cleanup branch April 12, 2019 15:57
@natemccurdy
Copy link
Contributor Author

natemccurdy commented Apr 12, 2019

@pahoughton-sf The lookup() function casts everything to a String. I can see that you are defining the port, 4464, as an Integer. If you switch to the alias() function, the data type will be preserved.

agate::port: 4464

agate::consul_svc:
  agate:
    port: "%{alias('agate::port')}"
    meta:
      example: value

Does that work for you?

If not, I could submit a PR that opens up the type restriction to allow Integers or Strings, but I'd prefer not to. Mainly because the standard type for a port range is generally an Integer: Integer[0, 65535]. This is based on the Stdlib::Port data type from puppetlabs/stdlib: https://github.com/puppetlabs/puppetlabs-stdlib/blob/master/types/port.pp

For reference, here's a type that allows integers and strings that look like numbers:
Variant[Integer[0,65535],Pattern[/\d+/]]

@pahoughton-sf
Copy link

The alias will work perfect for my use case. Thanks!

spuder pushed a commit to spuder/puppet-consul that referenced this pull request Feb 25, 2020
Lint, style, and parameter data types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants