search

voxpupuli / puppet-rundeck

Module for managing the installatation and configuration of the rundeck orchestration tool
https://forge.puppet.com/puppet/rundeck
MIT License
39 stars 130 forks source link

(#459) prevent colon added in realm.properties when auth_users hash is empty #460

Closed overag3 closed 3 years ago

overag3 commented 3 years ago

Pull Request (PR) description

This changeset fix issue when @auth_config['file']['auth_users'] hash is empty, a colon added in realm.properties file when it shouldn't. Fix also wrong indentation.

This Pull Request (PR) fixes the following issues

Fixes #459

overag3 commented 3 years ago

Test already exists in auth_spec.rb file but i found the real problem...

In params manifest(L62, L168), the type of auth_users variable is hash but in spec tests and in real.properties.erb template file, the type of auth_users needs to be a array not hash.

Besides, is line 62 necessary ?

kenyon commented 3 years ago

Good finds. Yeah, looks like auth_users in params.pp line 62 is unused. The template also treats it as a hash, in addition to an array: https://github.com/voxpupuli/puppet-rundeck/blob/8b257e4507eaafae349544080d9ee6ecfc664835/templates/realm.properties.erb#L35

This module needs better data typing and better testing. :unamused:

overag3 commented 3 years ago

Am I modifying the params.pp file in this PR ?

I have never written spec tests before. :worried:

overag3 commented 3 years ago

Good finds. Yeah, looks like auth_users in params.pp line 62 is unused. The template also treats it as a hash, in addition to an array:

https://github.com/voxpupuli/puppet-rundeck/blob/8b257e4507eaafae349544080d9ee6ecfc664835/templates/realm.properties.erb#L35

This module needs better data typing and better testing. unamused

Yes, auth_users is a array of hashes. Example is available in :

https://github.com/voxpupuli/puppet-rundeck/blob/d50ff9118dffd95ec43d51201813be2cdf2a5bcd/spec/classes/config/global/auth_spec.rb#L56-L67

Tell me what I can do to solve this issue.

Regards

kenyon commented 3 years ago

I think this PR is fine as is since it fixes the problem. We can make other PRs for overhauling the data types and testing. I'm just waiting for anyone else to provide feedback here, otherwise I think we can merge this soon.