Optimizing puppet var types

[Joris29]

Use Case

Previously i opened an issue with a type being wrong but i'm facing an other issue which can be resolved on my side but i think an overal review of the different new var types should be considered.

I'll try to describe some "wrong" types.

Describe the Solution You Would Like

To match var types better with their purpose

Additional Context

I'll add examples from where i would change types i will not include all params or go through all classes but just pasting some most used probably.

define tomcat::config::server (
Optional[String[1]] $address = undef, => since this has to be TCP/IP address check better instead of string
Optional[String[1]] $port = undef, => Change to integer or variant [string, integer]
Optional[String[1]] $server_config = undef, => Change to absolutepath since docs says "valid option string containing absolutepath"
) {

define tomcat::install (
String[1] $catalina_home = $name, => Change to absolutepath since docs says "valid option string containing absolutepath"
Optional[String[1]] $source_url = undef, => Change to enum or update docs this is not check but docs says "Valid options: a string containing a puppet://, http(s)://, or ftp://"
Optional[String[1]] $proxy_server = undef, => Could be checked better to since you have an enum on proxy_type
) {

This is ofc just a suggestion but stricter checking or using the correct types will help everyone in my opinion :)

[LukasAud]

Hi @Joris29, thank for pointing out these potential improvements. One of the primary reasons why we didn't use very strict types was to avoid a very abrupt backwards-incompatible update. Some changes of the mentioned changes can probably get away with a stricter datatype right away and we will look into it.

The $source_url suggestion, however, cannot be implemented as (afaik) the parameter is used to contain a full URL that starts with either puppet://, http(s):// or ftp://, rather than exclusively those three String values. You can see examples of this happening in our Spec test files. Maybe the wording of this parameter description could be adjusted to be a little less vague.

I cannot ensure either that the $address change will work as we seem to permit the value 'localhost' as a valid option, while our custom type Stdlib::IP::Address doesn't specify that as a true value.

We will investigate these and more possible parameter type updates soon. However, if there is any potential upgrade/bugfix that you need immediately merged into the module, feel free to drop a Pull Request and we will review/merge it as soon as we can.

[Joris29]

The one that bothers me most at the moment is the port being a string it just feels weird but I'll see if i can make a PR

[Joris29]

And about the address you could work with a variant either being Stdlib::IP::Address or localhost
Something like this:
Variant[Stdlib::IP::Address, 'localhost']

[LukasAud]

@Joris29 A limitation regarding our custom Port type is that it allows exclusively Integers. Which might break the builds of users that were passing Strings. That one might be have to be remade as a Variant[String[1], Stdlib::Port] to avoid breaking changes.

The $address suggestion is a fairly good idea. Can't believe I didnt think about it before 😅