Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update key storage parameters and update documentation #516

Merged
merged 2 commits into from Nov 10, 2023

Conversation

Joris29
Copy link
Contributor

@Joris29 Joris29 commented Oct 26, 2023

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 Joris29 force-pushed the patch-2 branch 13 times, most recently from 648047e to ae2cd4d Compare October 27, 2023 06:34
@Joris29
Copy link
Contributor Author

Joris29 commented Nov 6, 2023

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

Copy link
Member

@kenyon kenyon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Followup to #515.

This is a breaking change due to the init.pp parameter changes, so will need a major version bump.

Really need to convert the documentation comments to Puppet Strings format, remove params.pp, and remove all the reference from the readme.

@Joris29
Copy link
Contributor Author

Joris29 commented Nov 6, 2023

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

@kenyon
Copy link
Member

kenyon commented Nov 6, 2023

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

@Joris29
Copy link
Contributor Author

Joris29 commented Nov 8, 2023

@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
Copy link
Member

kenyon commented Nov 8, 2023

Yes, sounds good.

The key storage type parameter limits the key storage entries to one.
I 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.
@Joris29
Copy link
Contributor Author

Joris29 commented Nov 9, 2023

@kenyon PR rebased and updated

Copy link
Member

@kenyon kenyon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I don't use this module, and I know this would make code maintenance harder, but I wonder if it would be worthwhile to keep the old key_storage_type parameter for backwards compatibility.

@Joris29
Copy link
Contributor Author

Joris29 commented Nov 10, 2023

@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.

@kenyon kenyon merged commit 067283e into voxpupuli:master Nov 10, 2023
32 checks passed
@Joris29
Copy link
Contributor Author

Joris29 commented Nov 10, 2023

@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
Copy link
Member

kenyon commented Nov 10, 2023

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

@Joris29 Joris29 deleted the patch-2 branch December 13, 2023 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants