Module quality update

[Joris29]

Pull Request (PR) description

This PR will make several changes to follow the best practices of puppet more and remove deprecated parameters.

Like removing params.pp
Removing projects_storage_type parameter
...

This will be a breaking change for some use cases.

This Pull Request (PR) fixes the following issues

Fixes #197
Fixes #373
Makes #398 obsolete
Fixes #406
Fixes #426
Fixes #445
Makes #447 obsolete
Fixes #451
Fixes #452
Fixes #457
Fixes #472
Fixes #479
Fixes #484
Makes #487 obsolete
Makes #521 obsolete

[Joris29]

@kenyon Hello it's been a while, in my attempt to remove the params.pp i also took the opportunity to rework the module entirely.

The unit tests still need to updated and maybe some other things not related to the code.
The code has been tested on a RHEL 8 machine.

My main questions is how to progress towards a new release is this something i need to do or how would this be done?

[kenyon]

Looks pretty good.

We like to have parameter defaults documented in REFERENCE.md as much as possible, so move everything from common.yaml to init.pp.

Don't put anything in spec/fixtures, the test code automatically copies everything there.

I would avoid aligning the = signs in class parameter lists, since it causes lots of diffs which makes changes harder to review, and can get hard to read when lines get lengthy.

As far as a new release, it's the usual procedure: the change is merged into the master branch, and eventually we make a new major release.

[Joris29]

Good to know nothing has to be in spec/fixtures

I will try to move some things from common but what about big hashes like the admin policy and the os family specific things?

[kenyon]

OS-specific things should still go in the OS data file in hiera. I think big hashes in init.pp are fine, this allows them to be documented in REFERENCE.md.

[Joris29]

I moved the parameter values to init.pp but i'm having some difficulties with framework_config.

This should be merged with the defaults from rundeck because if you overwrite the framework_config paramter then you lose all defaults if you don't set them explicitly like rdeck.base and i would like to avoid this so what's the best approach for this?

[kenyon]

I moved the parameter values to init.pp but i'm having some difficulties with framework_config.

This should be merged with the defaults from rundeck because if you overwrite the framework_config paramter then you lose all defaults if you don't set them explicitly like rdeck.base and i would like to avoid this so what's the best approach for this?

One thing you can do is make $framework_config only override the defaults. Set the defaults in the code, then merge (+) the hashes.

[Joris29]

@kenyon I updated the specs and references even further for me everything checks out one check fails but it has nothing to do with the actual unit tests. The only thing still on my todo is to update the readme as this includes wrong example for now.
Feel free to add more todo's if needed i also updated which issues/PR's will be fixed or become obsolete i might have forgot some but this can resolved later i guess?

[kenyon]

Looks pretty good so far.

[Joris29]

@kenyon Update the PR for ubuntu and the unit tests all check out.

[kenyon]

Looks good.

Do you want to squash the commits?

Would it be helpful to add a section to the README about how to migrate to this new version of the module from the current version?

[Joris29]

Yeah maybe it's best to squash the commits as there are quite a lot and some commits are not really clear on what they do exactly.

For the migration part i think this will kind off be part of the change logs because all the breaking changes will be in there.
There is also not much to it except renaming/removing parameters or restructuring some data like the auth_config or key_storage_config but examples for both are added to the readme.

[Joris29]

Should i do the squash now or will you do it when merging?

[kenyon]

Release PR #523 includes this change.

[Fabian1976]

I'm sorry to say that this pull request actually reduced the quality of the module.

This parameter:

dataSource.dbCreate = update

Found on this line, is now hard-coded in the module and prevents new installation of rundeck and a postgresql backend.

In the version before this change (v8.0.1), it was a proper parameter:

dataSource {
  dbCreate = "<%= @database_config['dbCreate'] %>"

Because of that fixed parameter, new installs of rundeck with Postgresql generate this error:

[2022-09-29T21:22:05,759] ERROR boot.SpringApplication - Application run failed
liquibase.exception.LiquibaseException: liquibase.exception.MigrationFailedException: Migration failed for change set core/ExecReportJcExecIdToExecutionId.groovy::1653344096108-52::gschueler (generated):
     Reason: liquibase.exception.DatabaseException: ERROR: column "jc_exec_id" does not exist
  Position: 44 [Failed SQL: (0) update base_report set execution_id = cast(JC_EXEC_ID as bigint)]

Manually removing this parameter results in rundeck starting just fine.

I will also log an issue for this.