Module params.pp and parametrised variables...

[Gavin Williams]

Morning all

I'm trying to add some functionality to my Puppet-Glassfish[1] module to support providing an alternative download mirror.

I've made the initial code changes in caa445631d[2], however a couple of my tests are now failing[3]. 

I believe the issue is due to using the '$glassfish::version' variable within my params.pp[4] file which is also exposed as class variables[5] which can then be over-ridden on calling. 

I've tried using both the fully qualified '$glassfish::version' variable, aswell as just the local '$version' variable, but neither seem to work. 

The tests are complaining that the generated '$glassfish::glassfish_download_mirror' variable doesn't include a version number:

expected that the catalogue would contain Exec[download_glassfish-3.1.2.2.zip] with command set to "wget -q http://download.java.net/glassfish/3.1.2.2/release/glassfish-3.1.2.2.zip -O /tmp/glassfish-3.1.2.2.zip" but it is set to "wget -q http://download.java.net/glassfish//release/glassfish-3.1.2.2.zip -O /tmp/glassfish-3.1.2.2.zip"

Note the double '//' between glassfish and release.

Should this pattern work? 

Or how should I be doing it? 

Cheers

Gavin 

[1] https://github.com/fatmcgav/fatmcgav-glassfish/tree/feature/alternate_download

[2] https://github.com/fatmcgav/fatmcgav-glassfish/commit/caa445631d5662d5d150785a8949d5c554595007

[3] https://travis-ci.org/fatmcgav/fatmcgav-glassfish/jobs/46712102#L1200

[4] https://github.com/fatmcgav/fatmcgav-glassfish/blob/caa445631d5662d5d150785a8949d5c554595007/manifests/params.pp#L25

[5] https://github.com/fatmcgav/fatmcgav-glassfish/blob/caa445631d5662d5d150785a8949d5c554595007/manifests/init.pp#L121

[jcbollinger]

On Monday, January 12, 2015 at 5:00:04 AM UTC-6, Gavin Williams wrote:

Morning all

I'm trying to add some functionality to my Puppet-Glassfish[1] module to support providing an alternative download mirror.

I've made the initial code changes in caa445631d[2], however a couple of my tests are now failing[3]. 

I believe the issue is due to using the '$glassfish::version' variable within my params.pp[4] file which is also exposed as class variables[5] which can then be over-ridden on calling. 

I've tried using both the fully qualified '$glassfish::version' variable, aswell as just the local '$version' variable, but neither seem to work. 

The tests are complaining that the generated '$glassfish::glassfish_download_mirror' variable doesn't include a version number:

expected that the catalogue would contain Exec[download_glassfish-3.1.2.2.zip] with command set to "wget -q http://download.java.net/glassfish/3.1.2.2/release/glassfish-3.1.2.2.zip -O /tmp/glassfish-3.1.2.2.zip" but it is set to "wget -q http://download.java.net/glassfish//release/glassfish-3.1.2.2.zip -O /tmp/glassfish-3.1.2.2.zip"

Note the double '//' between glassfish and release.

Should this pattern work? 

Or how should I be doing it? 

I'm not sure what you mean by "this pattern", but it is certainly system-dependent whether the URL you are now producing refers to the same underlying physical resource as the one your tests expect, or indeed, whether it can possibly refers to any physical resource at all.  You should certainly fix the problem highlighted by the tests (and kudos to you for having tests that picked this up!).

In my experience, a double slash in a path normally arises one of two ways:

1. from concatenating a path ending in a slash with one beginning with a slash, or
2. from joining a two strings with a '/' delimiter, when the first one already ends with a slash or the second begins with one.

Supposing that one of those is what's happening here, the solution is probably to be very clear about what each parameter and variable represents, to provide only valid values for them, and to perform on them only operations that are correct for those entities.

Specifically, if you have a variable containing the value "/release/glassfish-3.1.2.2.zip", then what, exactly, is that variable supposed to represent?  It is not a relative path, but apparently it's not a correct absolute path, either.  I can't think of what a variable might represent in your module for such a value to be valid for it, so if one of your variables has such a value then that is probably incorrect.  Alternatively, if you have a variable expected to contain a valid directory URL (which must end in a '/'), and in fact containing the valid directory URL "http::/java.download.net/glassfish/", and you join the value of that variable to a relative path, then you should not put a '/' between the parts because you can rely on the base URL already containing one.

To the extent that you may not consider data fed to your module from outside to be completely reliable, you should consider implementing validation checks.


John

[Gavin Williams]

John

Cheers for the response. 

I guess I could have explained the issue a bit better :) 

By pattern, I'm referring to using a params.pp class with variable interpolation... I.e.

# --glassfish/manifests/params.pp#25

 $glassfish_download_mirror = "http://download.java.net/glassfish/${glassfish::version}/release"

This var is then used as the default for:

 # --glassfish/manifests/init.pp#102

 $download_mirror = $glassfish::params::glassfish_download_mirror,

'$glassfish::version' is defaulted in glassfish/manifests/params.pp#37 to '3.1.2.2', and exposed as a class param in glassfish/manifests/init.pp#121, allowing users to plug-in other versions. 

The download URL is then put together in glassfish/manifests/install.pp#79. However the issue with the failing tests is that ${glassfish::version} above is evaluating to an empty string, resulting in a download path of "http://download.java.net/glassfish//release/glassfish-3.1.2.2.zip". In-between the double '/' should be the $version value.

What I was aiming for is to have a default mirror that looks at java.net and uses the passed in ${glassfish::version} value, but allow people to plug in an internal mirror URL, which likely won't have any variables in it. 

So when running with defaults, the above URL should be as per the test expectation...  

Cheers

Gav

[jcbollinger]

On Monday, January 12, 2015 at 9:42:50 AM UTC-6, Gavin Williams wrote:

John

Cheers for the response. 

I guess I could have explained the issue a bit better :) 

By pattern, I'm referring to using a params.pp class with variable interpolation... I.e.

# --glassfish/manifests/params.pp#25

 $glassfish_download_mirror = "http://download.java.net/glassfish/${glassfish::version}/release"

This var is then used as the default for:

 # --glassfish/manifests/init.pp#102

 $download_mirror = $glassfish::params::glassfish_download_mirror,

'$glassfish::version' is defaulted in glassfish/manifests/params.pp#37 to '3.1.2.2', and exposed as a class param in glassfish/manifests/init.pp#121, allowing users to plug-in other versions. 

So no, that cannot work.  You have established an unresolvable evaluation-order dependency cycle.  Variable $glassfish::params::glassfish_download_server of class glassfish::params depends on variable $glassfish::version of class glassfish, so class glassfish must be evaluated first to get the desired value.  At the same time, parameter $glassfish::download_mirror of class glassfish depends on variable $glassfish::params::glassfish_download_mirror of class glassfish::params for its default value, so class glassfish::params must be evaluated first (must be inherited from, in fact) for that to work.  You cannot have it both ways.

 

The download URL is then put together in glassfish/manifests/install.pp#79. However the issue with the failing tests is that ${glassfish::version} above is evaluating to an empty string, resulting in a download path of "http://download.java.net/glassfish//release/glassfish-3.1.2.2.zip". In-between the double '/' should be the $version value.

So that's a *third* way of getting that double slash.  I guess I overlooked that one.

You are being very parsimonious with your code.  Although I appreciate you not slamming us with a code mountain, it really would have been -- and may still be -- helpful to present at least a stripped-down form of the two complete classes at issue.

In any case, this seems a bit of a muddle.  In particular, the role of class glassfish::params seems a bit unfocused.  A params class generally should stick to providing data, but yours also applies policy, at least that "the default download URL for any version shall be formed by inserting the version string into a URL template of the following format". There is no bright line separating data from policy, but you have certainly gone too far when your params class depends on values that are intended to be open for client override.

 

What I was aiming for is to have a default mirror that looks at java.net and uses the passed in ${glassfish::version} value, but allow people to plug in an internal mirror URL, which likely won't have any variables in it. 

So when running with defaults, the above URL should be as per the test expectation...  

That's totally achievable; you just need to put the logic in the right place.  If you intend to use variable $glassfish::download_mirror only in class glassfish::install, then I think I'd put that logic in glassfish::install.  Otherwise, it should go in class glassfish.  For example,

class glassfish (
    $version = $glassfish::params::version,
    $download_mirror = undef
) inherits glassfish::params {
  # ...
}

class glassfish::install {
  include 'glassfish'
 
  $archive_name = "glassfish-${glassfish::version}.zip"
  $download_dir_url = "$glassfish::download_mirror" ? {
    '' => "http://download.java.net/glassfish/${glassfish::version}/release/",
    default => $glassfish::download_mirror
  }
  $download_url = "${download_dir_url}${archive_name}"
  # ...
}

John

[Gavin Williams]

John

Cheers again for the response... 

I had a feeling that the issue was due to evaluation order of params etc... 

Moving the logic into glassfish::install has had the desired outcome, and all my tests now pass :) 

The changes I made are here: https://github.com/fatmcgav/fatmcgav-glassfish/commit/172d87f9386699957fb409ab3ec913789227aaa5

Cheers again. 

Gav