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

Declare minimum Puppet version to be 6.24.0 #2342

Merged
merged 1 commit into from
Nov 14, 2022

Conversation

ekohl
Copy link
Collaborator

@ekohl ekohl commented Nov 14, 2022

In a565ea9 the commands are passed as an array, but this feature was only introduced in Puppet 6.24.0. This raises the minimum version to match, since it's no longer possible to use the module on anything older.

Fixes #2328

In a565ea9 the commands are passed as
an array, but this feature was only introduced in Puppet 6.24.0[1]. This
raises the minimum version to match, since it's no longer possible to
use the module on anything older.

[1]: https://puppet.com/docs/puppet/6/release_notes_puppet.html#enhancements_puppet_x-7-9-0-PUP-5704

Fixes: a565ea9
@trefzer
Copy link
Contributor

trefzer commented Nov 14, 2022

I could not figure out if puppet minimal requirement declared in the metadata file is for the version of puppetserver or the puppetclient (anyone knows ?).
The exec version changed in the puppet client. I think we should probably add a fail message in the puppet code if client is below 6.24 to lead people in the correct direction what they need to update !
(I know several installations with puppetserver > 6 but still clients <6 !!)

@david22swan
Copy link
Member

Agree on the change and would like it merged in, however will need to wait as there is still some discussion on our side and other work that we would want in alongside this as part of a major release.

@ekohl
Copy link
Collaborator Author

ekohl commented Nov 14, 2022

I could not figure out if puppet minimal requirement declared in the metadata file is for the version of puppetserver or the puppetclient (anyone knows ?).

I'd say at least the agent since it's perfectly valid to apply a module locally without a Puppetserver present. Additionally, it relies on types and providers supplied by the agent. As a last point, it has been a recommendation that Puppetserver >= agent so this does imply the server needs to be reasonably new.

However, AFAIK nothing formally checks this requirement programmatically. This is why Kafo (the library driving Foreman's installer) implements it

Agree on the change and would like it merged in, however will need to wait as there is still some discussion on our side and other work that we would want in alongside this as part of a major release.

I disagree this is breaking. It is just being explicit what was already implicit. If you think this is breaking, we should revert a565ea9 now and reintroduce it in a new major version.

@chelnak
Copy link
Contributor

chelnak commented Nov 14, 2022

FWIW I don't want to revert changes that are already in main and released.

I'd much rather us fix forward.

@david22swan
Copy link
Member

I'm not sure that reverting is an option at this point as this is something that has been implemented across several modules, with multiple of them having undergone a release since.
In regards to releasing it as a non-major change........
While I don't particularly like it, I suppose the alternative would involve pulling the last release which is something that I enjoy less.

@david22swan
Copy link
Member

Yeah, we'll go ahead with a non major release.
It's the least bad option of a few bad options.

@david22swan
Copy link
Member

@ekohl Thanks for putting up the PR

@david22swan david22swan merged commit 0aac6ba into puppetlabs:main Nov 14, 2022
@ekohl ekohl deleted the update-puppet-version branch November 14, 2022 16:54
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.

Incorrect Type Passed to confd_command in init.pp
5 participants