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

Bump min version of stdlib #681

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

waipeng
Copy link
Contributor

@waipeng waipeng commented May 28, 2024

Summary

PR #637 uses stdlib::deferrable_epp that is only available in v8.4.0[1]

[1] puppetlabs/puppetlabs-stdlib#1253

Checklist

  • 🟢 Spec tests.
  • 🟢 Acceptance tests.
  • Manually verified. (For example puppet apply)

PR puppetlabs#637 uses stdlib::deferrable_epp that is only available in v8.4.0[1]

[1] puppetlabs/puppetlabs-stdlib#1253

Change-Id: I455cec0ec1c6a2ec63b2ab0c19b2b7a2119f2f26
@waipeng waipeng requested review from bastelfreak, deric and a team as code owners May 28, 2024 06:56
@bastelfreak
Copy link
Collaborator

I'm not sure if that's the right move. I think instead we should move stdlib >= 9.

https://github.com/puppetlabs/puppetlabs-kubernetes/blob/main/manifests/config/kubeadm.pp#L369-L370 for example still uses the deprecated merge() method: https://github.com/puppetlabs/puppetlabs-stdlib/blob/main/lib/puppet/functions/merge.rb

And that fails on puppet 8 with strict mode on. I think that metadata.json claims stdlib 9 + Puppet 8 compatibility isn't correct at the moment. The deprecated function calls should be removed and the stdlib lower version should be raised to 9.

@deric
Copy link
Collaborator

deric commented May 28, 2024

@bastelfreak I think bumping from 4 -> 8 is the right move right now. It would be nice to prepare at least a minor version with all those changes. Moving to stdlib >= 9 could be done in next PR, marked as a major version release.

@bastelfreak
Copy link
Collaborator

I think raising the stdlib version always justifies a major releases. That's how we usually did it in the past. And when you bump the lower version to 8 I think you should lower the maximum version, so basically >= 8 < 9. because it doesn't really work with Puppet 8/stdlib 9.

@deric
Copy link
Collaborator

deric commented May 28, 2024

The lower stdlib version change should have been done in 9f9f0eb, that is part of v8.0.0 release. It makes sense to fix that issue in v8.x branch.

However, if you would prepare 9.0.0 release with stdlib >= 8 < 9 requirement, I'm ok with that.

@waipeng
Copy link
Contributor Author

waipeng commented May 29, 2024

I agree with @deric , think of this PR as fixing a bug in the lower constraint for stdlib
The issue that @bastelfreak reported will be another bug in the upper constraint for stdlib

I wonder if there's a good way to test for these kind of issues?

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.

4 participants