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

switch to stdlib namespaced functions, require stdlib 9.x #699

Merged
merged 3 commits into from
Sep 22, 2023

Conversation

tuxmea
Copy link
Member

@tuxmea tuxmea commented Sep 21, 2023

No description provided.

manifests/dev.pp Show resolved Hide resolved
@evgeni
Copy link
Member

evgeni commented Sep 21, 2023

(the tests install puppetlabs-stdlib (v8.6.0) which explains why it was not caught during bumping)

@tuxmea tuxmea requested a review from evgeni September 21, 2023 07:59
@evgeni
Copy link
Member

evgeni commented Sep 21, 2023

@bastelfreak would this be considered a breaking change?

technically it's fixing a bug introduced by #691 which allowed stdlib 9.x without using the new namespaced name (insert raising fist emoji), so I'd lean to "no", but requirement bumps always give me headaches.

Edit:

OTOH, looking at CI logs, 9.3.0 has a non-namespaced version, and things did work with it:

2023-09-21T07:55:17.3931405Z �[00;00m�[00;33m  Warning: This function is deprecated, please use stdlib::ensure_packages instead. at ["/etc/puppetlabs/code/environments/production/modules/php/manifests/pear.pp", 50]:["/tmp/apply_manifest_075508438.pp.8vjBav", 1]
2023-09-21T07:55:17.3933171Z      (location: /etc/puppetlabs/code/environments/production/modules/stdlib/lib/puppet/functions/deprecation.rb:35:in `deprecation')

So it was not a bug as such?

@evgeni
Copy link
Member

evgeni commented Sep 21, 2023

@tuxmea I guess the following places should also be updated?

ensure_packages(['wget'])

ensure_packages(["${php::package_prefix}xml"], { ensure => present, require => $require, })

@tuxmea
Copy link
Member Author

tuxmea commented Sep 21, 2023

@tuxmea I guess the following places should also be updated?

ensure_packages(['wget'])

ensure_packages(["${php::package_prefix}xml"], { ensure => present, require => $require, })

Done

@evgeni evgeni changed the title switch to stdlib namespaced functions switch to stdlib namespaced functions, require stdlib 9.x Sep 21, 2023
Copy link
Member

@evgeni evgeni left a comment

Choose a reason for hiding this comment

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

I'm fine with the change, but unsure how to label that in terms of bug/enhancement/breaking.

I thought stdlib 9.x was breaking the old, non-namespaced calling of ensure_packages (which would make this a bugfix), but it seems there is a shim and it works fine (emitting a deprecation warning), making this either an enhancement or a breaking change (depending on "is bumping a req a breaking change"). I lean towards enhancement.

@tuxmea
Copy link
Member Author

tuxmea commented Sep 21, 2023

As ensure_packages is available (and shows a deprecation) I would consider this to not be a breaking change. But I am open to other opinions. @bastelfreak what do you prefer?

@@ -11,7 +11,7 @@
"dependencies": [
{
"name": "puppetlabs/stdlib",
"version_requirement": ">= 4.16.0 < 10.0.0"
"version_requirement": ">= 9.0.0 < 10.0.0"
Copy link
Member

Choose a reason for hiding this comment

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

Changing the lower version is a breaking change (e.g. users of version 7.0.0 will have unmet dependencies when updating, and ignoring it would break their catalog as the namespaced function is not available).

Copy link
Member

@smortex smortex left a comment

Choose a reason for hiding this comment

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

LGTM!

@tuxmea tuxmea merged commit af276e1 into voxpupuli:master Sep 22, 2023
16 of 17 checks passed
@tuxmea tuxmea deleted the stdlib_namespaced_functions branch September 22, 2023 07:38
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.

5 participants