schrepfler / puppet-jdk_oracle

Puppet module to install JDK from oracle using wget
Other
1 stars 7 forks source link

Enhancement: hiera lookup not required as this is default since puppet 3 #8

Closed 030 closed 7 years ago

030 commented 8 years ago
  $version      = hiera('jdk_oracle::version',        '8' ),
  $arch         = hiera('jdk_oracle::arch',           'x64' ),
  $install_dir  = hiera('jdk_oracle::install_dir',    '/usr/java' ),
  $tmp_dir      = hiera('jdk_oracle::tmp_dir',        '/tmp' ),
  $use_cache    = hiera('jdk_oracle::use_cache',      false ),
  $cache_source = 'puppet:///modules/jdk_oracle/',
  $jce          = hiera('jdk_oracle::jce',            false ),
  $default_java = hiera('jdk_oracle::default_java',   true),

could be changed to

  $version      =   8 ,
  $arch         =  'x64' ,
  $install_dir  =   '/usr/java',
  $tmp_dir      =  '/tmp',
  $use_cache    =  false ,
  $cache_source = 'puppet:///modules/jdk_oracle/',
  $jce          = false,
  $default_java = true,
schrepfler commented 8 years ago

I didn't know this, good change but what are the implications on compatibility though? How would the module behave pre-puppet 3? How would the version be passed?

030 commented 8 years ago

That is a good question. Users that use an old version of Puppet are restricted to use 0.9.7 and if puppet4 is used then users could use newer version. We could also check if the version of puppet is 4.

$supported_puppetversion = '4.0.0'

if !($::puppetversion > $supported_puppetversion) {
    fail("Incompatible with \"${::puppetversion}\". Greater than: \"${supported_puppetversion}\" is required")
}
schrepfler commented 8 years ago

I guess this is philosophical, my thoughts are that compatibility should trump the reduction in code. What are your thoughts, should we leave it to do the hiera lookup anyhow?

030 commented 8 years ago

Tested using the master branch:

hiera

jdk_oracle::version: 6

the default 8 is overwritten (without using hiera() lookup function)

Error: Evaluation Error: Error while evaluating a Function Call, Unsupported version: 6. Supported versions are 7 and 8 at ../jdk_oracle/manifests/init.pp:62:7 on node localhost
030 commented 8 years ago

I will have to do some more research regarding the compatibility whether it is seen as a minor, intermediate or major release.

schrepfler commented 8 years ago

How would you propose we move forward, bump major version and have changelog with entry stating breaking change?

030 commented 8 years ago

Sounds like a good idea. If people would like to upgrade then they have to check the changelog and then they will be informed.

schrepfler commented 8 years ago

So, just to be clear, if we have this change puppet 3 support goes down the drain, there's no way we can make it compatible with both?

schrepfler commented 8 years ago

I've noticed post publishing that only the version got actually changed in the commit?

030 commented 8 years ago

People who would like to use an older Puppet version should not update

schrepfler commented 8 years ago

I've managed to install the module on both puppet 3 and puppet 4 boxes with a simple import statement. My guess is this will fail only if somebody is explicitly setting any of the parameters or injecting them via hiera (can you confirm this?) I'm of the thought we should leave the hiera lookup syntax for all variables, I think that will make it puppet3 compatible yet still work with puppet4. It's a minor nuisance for us to have these 5-6 lines of code but more people will be able to continue using the module?

schrepfler commented 7 years ago

Needs confirmation from people using the module using hiera.