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 128 forks source link

Update key storage parameters and update documentation #516

Closed Joris29 closed 1 year ago

Joris29 commented 1 year ago

Pull Request (PR) description

The key storage type parameter limits the key storage entries to one. It would be better to simplify this to one configurable hash. Also remove vault specific parameters to limit breaking changes when vault-storage plugin updates config on their side. Update docs as well.

Joris29 commented 1 year ago

@kenyon A follow up PR due to limitations in the template could you take a look thanks in advance

Joris29 commented 1 year ago

Alright do i need to do some additional steps myself regarding the puppet strings and the other things you mentioned?

kenyon commented 1 year ago

@Joris29 it would be great if you want to do the work. Separate pull requests for those changes would be good.

Joris29 commented 1 year ago

@kenyon I will update this PR according to my other merged PR but can this PR be merged before opening another PR to remove the params.pp because it will be quite a big change when removing params.pp?

kenyon commented 1 year ago

Yes, sounds good.

Joris29 commented 1 year ago

@kenyon PR rebased and updated

Joris29 commented 1 year ago

@kenyon I use this module for different rundeck environments and i worked quite a bit with this module. My honest opinion is that it's not worthwhile especially not when looking to maintain the code.

My change introduces a new more dynamic way of configuring the storage provider/type while previous implementation adds every option as a parameter so that also means if new params are introduced or get renamed (See PR for vault) things might brake or a new PR has to be created for every change from rundeck or the storage plugin.

While it will be possible to add both options i think it will only add more complexity, if both are set by accident or if none are set those are all things that would have to be added and tested.

About the backwards incompatibility i agree it's not ideal but i also think in a module's lifetime somewhere this is inevitable. If it's documented and added as breaking change i don't see why we can't merge, older versions can still be used.

Joris29 commented 1 year ago

@kenyon Thanks for merging now for the follow up, do you want to create a new release first because that's not something i can do i guess or do i first create a PR to remove the params.pp and then create a new release?

kenyon commented 1 year ago

@Joris29 might as well get more changes into the next release, like params.pp removal.