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

Ignore Puppet's strict setting when calling function without namespace #1377

Merged
merged 1 commit into from Jun 27, 2023

Conversation

alexjfisher
Copy link
Collaborator

@alexjfisher alexjfisher commented Jun 23, 2023

Previously, when a user had the Puppet setting strict set to error (which is the default in Puppet 8), a call to one of stdlib's functions via the deprecated non-namespaced function would cause a hard failure instead of just logging a warning and calling the real namespaced function.

In this change, all of our shims have been updated to call deprecation with its new third parameter, (use_strict_setting), set to false. The non-namespaced versions will now only ever log warnings informing users to moved to the namespaced versions. It will not raise exceptions even if strict is set to error.

This change will make it much easier for users to migrate to stdlib 9 (and to upgrade modules that now depend on stdlib 9)

Fixes #1373

(Note, this PR description was updated after the deprecation function update was extracted into its own PR #1378)

@chelnak
Copy link
Contributor

chelnak commented Jun 23, 2023

I don't feel strongly either way but I wonder if it would be cleaner to have the implementation and updates should be in separate PRs?

Copy link

@jhoblitt jhoblitt left a comment

Choose a reason for hiding this comment

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

I think this is probably the least painful path forward from the current situation. I hope that in the future we will do a better job of not introducing a deprecation and the replacement facility in the same release. Otherwise, strict = error will quickly become meaningless and/or users will change to strict = warn.

ekohl
ekohl previously approved these changes Jun 26, 2023
Copy link
Collaborator

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Makes sense to me.

lib/puppet/functions/deprecation.rb Outdated Show resolved Hide resolved
@chelnak
Copy link
Contributor

chelnak commented Jun 26, 2023

How's everyone feeling about this today?

@bastelfreak
Copy link
Collaborator

I don't feel strongly either way but I wonder if it would be cleaner to have the implementation and updates should be in separate PRs?

I agree with @chelnak . @alexjfisher what do you think?

@alexjfisher
Copy link
Collaborator Author

@chelnak @bastelfreak I've split this into 2 commits and opened #1378 with just the updates to the deprecation function. If you approve and merge that one first, this one can go in second, and I'll update the description to match the single commit.

@chelnak
Copy link
Contributor

chelnak commented Jun 27, 2023

Thank you!

@chelnak
Copy link
Contributor

chelnak commented Jun 27, 2023

This PR should be ready for a rebase now!

chelnak
chelnak previously approved these changes Jun 27, 2023
@alexjfisher alexjfisher dismissed chelnak’s stale review June 27, 2023 11:42

The merge-base changed after approval.

chelnak
chelnak previously approved these changes Jun 27, 2023
@alexjfisher alexjfisher dismissed chelnak’s stale review June 27, 2023 11:50

The merge-base changed after approval.

chelnak
chelnak previously approved these changes Jun 27, 2023
@chelnak chelnak requested a review from bastelfreak June 27, 2023 11:51
@alexjfisher alexjfisher dismissed chelnak’s stale review June 27, 2023 11:58

The merge-base changed after approval.

Previously, when a user had the Puppet setting `strict` set to `error` (which
is the default in Puppet 8), a call to one of stdlib's functions via the
deprecated non-namespaced function would cause a hard failure instead of just
logging a warning and calling the real namespaced function.

In this change, all of our shims have been updated to call `deprecation` with
its new third parameter, (`use_strict_setting`), set to `false`. The
non-namespaced versions will now only ever log warnings informing users to
moved to the namespaced versions. It will not raise exceptions even if `strict`
is set to `error`.

This change will make it much easier for users to migrate to stdlib 9 (and to
upgrade modules that now depend on stdlib 9)

Fixes puppetlabs#1373
@alexjfisher alexjfisher requested a review from chelnak June 27, 2023 12:48
@chelnak chelnak merged commit 904b395 into puppetlabs:main Jun 27, 2023
44 checks passed
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.

Calls to Deprecation function cause catalog compilation to fail if strict setting is set to error
7 participants