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.

Sign up for GitHub

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

Jump to bottom

(CONT-801) Puppet 8 support / Drop Puppet 6 support #1307

Merged
merged 33 commits into from
May 24, 2023
Merged

(CONT-801) Puppet 8 support / Drop Puppet 6 support #1307

merged 33 commits into from
May 24, 2023

Conversation

LukasAud
Copy link
Contributor

@LukasAud LukasAud commented Apr 21, 2023 •
edited
Loading

This PR aims to implement support for Puppet 8. Some of the changes performed might result in loss of compatibility for Puppet 6, which we no longer support. Current changes implemented in this PR:

  • Pinned agent to latest version (This brings Puppet 8 into the formula, alongside Ruby 3.2)
  • Updated version requirements in metadata.json
  • Bumped Rubocop gems
  • Adjusted Rubocop configuration
  • Resolved new Rubocop offenses
  • Deprecated uriescape.rb and parsepson.rb

Issues addressed:

  • .scope is no longer a PuppetInternals functions. Now there is a new version living in RSpec. Addressed related testing.
  • srand() returns a different sort after Ruby 3 while inputting the same seed.
  • uriescape.rb no longer works past Ruby 3. Limiting testing to Puppet 7.
  • parsepson.rb is no longer supported as of Puppet 8 due to removal of native PSON support. Limiting testing to Puppet 7.
  • Found and addressed an issue related to positional vs keyword arguments in loadjson.rb

Note: Various issues found during testing were addressed by external PRs. Check PR history nearing this Request by backwards incompatible labels.

@alexjfisher
Copy link
Collaborator

Any chance we can we get a final 8.x release in with #1300 included first please?
cc @binford2k

@LukasAud
Copy link
Contributor Author

Hey @alexjfisher. Sure, we always aim to get a pre-release sorted before releasing any major update. Will take a deeper look into that PR once this update is finished and before we start the releasing new versions. It might, however, be hard to merge given the current state of the unit tests (got a few of them broken, trying to troubleshoot at the moment). Will let you know if we need a rebase on the other PRs end with our fix.

@alexjfisher
Copy link
Collaborator

@LukasAud #1308 should help with some of the current unit test failures.

@alexjfisher
Copy link
Collaborator

In fact it fixes all of the current unit test failures. (There are quite a few 'pending' tests that originally confused me when looking at the failed job logs.)

alexjfisher
alexjfisher reviewed Apr 24, 2023
View reviewed changes
.rubocop_todo.yml Outdated Show resolved Hide resolved
@alexjfisher
Copy link
Collaborator

Is this also a good opportunity to dump some of the deprecated functions? (Some of these could probably go now even without a major version bump)

@LukasAud
Copy link
Contributor Author

@alexjfisher Yes, already mentioned but we will take a look around and invite everyone else too to help us find and remove any deprecated functions along with their test cases. A release has been cut very recently so these PRs should be mergeable right away.

@jordanbreen28 jordanbreen28 force-pushed the CONT-801-Puppet_8_support branch 4 times, most recently from 9a5d45e to 1e494c1 Compare April 26, 2023 13:02
@LukasAud LukasAud force-pushed the CONT-801-Puppet_8_support branch 5 times, most recently from 1fdf6da to 25519ee Compare May 3, 2023 13:54
@chelnak chelnak force-pushed the CONT-801-Puppet_8_support branch from 26fbc9e to 8c82288 Compare May 10, 2023 15:16
@pmcmaw pmcmaw mentioned this pull request May 12, 2023
Closed
@LukasAud LukasAud force-pushed the CONT-801-Puppet_8_support branch 8 times, most recently from 3163631 to 2fcea98 Compare May 16, 2023 16:28
@LukasAud
Addressing uriescape testing
39c486b
@LukasAud
Addressing legacy facts
660b063
@LukasAud
(CONT-801) Safe correct Layout/ClosingHeredocIndentation
ec677d8
@LukasAud
(CONT-801) Autocorrect safe group 1
42a1f1a
@LukasAud
(CONT-801) Autocorrect safe group 2
7104469
@LukasAud
(CONT-801) Autocorrect safe group 3
303d7b0
@LukasAud
(CONT-801) Correct RSpec/EmptyLineAfterExample
76b40d3
@LukasAud
(CONT-801) Autocorrect safe group 4
0298868
@LukasAud
(CONT-801) Correct RSpec/ImplicitSubject
773e26d
@LukasAud
(CONT-801) Autocorrect safe group 5
19e72dc
@LukasAud
(CONT-801) Autocorrect safe group 6
8215008
@LukasAud
(CONT-801) Autocorrect safe group 7
e2c3f6a
@LukasAud
(CONT-801) Autocorrect safe group 8
e1a809d
@LukasAud
(CONT-801) Fix cop error
8840b86
@LukasAud
(CONT-801) Autocorrect unsafe group 1
18a8332
@LukasAud
(CONT-801) Unsafe autocorrect RSpec/BeEq
2d78851
@LukasAud
(CONT-801) Autocorrect unsafe group 2
0e3e5b1
@LukasAud
(CONT-801) Autocorrect unsafe group 3
55c4aea
@LukasAud
(CONT-801) Deprecate parsepson.rb
c0c7ccb
@LukasAud
(CONT-801) Deprecate uriescape.rb
799d608
@LukasAud
Reverting openURI changes
cab7d7e
david22swan
david22swan approved these changes May 24, 2023
View reviewed changes
Copy link
Member

@david22swan david22swan left a comment

Choose a reason for hiding this comment

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

LGTM

@LukasAud LukasAud merged commit d79b6d0 into main May 24, 2023
@LukasAud LukasAud deleted the CONT-801-Puppet_8_support branch May 24, 2023 10:53
traylenator added a commit to traylenator/puppetlabs-stdlib that referenced this pull request Oct 17, 2023
@traylenator
Revert "(CONT-801) Deprecate uriescape.rb"
6da5a2f 
This reverts commit 799d608.

While uriescape was deprecated for Puppet 8 in puppetlabs#1307
it was already fixed earlier for Puppet 8 and ruby 3 in puppetlabs#1195

It is unclear to me why this function was deprecated.

* Fixes puppetlabs#1401
@traylenator traylenator mentioned this pull request Oct 17, 2023
Open
alexjfisher
alexjfisher reviewed Feb 14, 2024
View reviewed changes
lib/puppet/functions/to_json_pretty.rb
# Call ::JSON to ensure it references the JSON library from Ruby's standard library
# instead of a random JSON namespace that might be in scope due to user code.
::JSON.pretty_generate(data, opts) << "\n"
JSON.pretty_generate(data, opts) << "\n"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder why these leading colons were removed? It doesn't seem to be a rubocop violation, and the comment above said they were there for a reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like it got 'safely' autocorrected by Rubocop during the Puppet 8 update. This might have been one of those cases where we should have added an inline exception instead of allowing it, but it probably sneaked past us.

I'll raise a PR to revert this and let the Modules team know about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just for context, the Puppet 8 update period was a fairly chaotic point where we were handling a lot of module changes simultaneously. At the same time, work leading to Puppet 8 also required us to clean up previous Lint exceptions living in the code for most modules, and one of those exceptions had to do with the usage of top-scope variable calling, so it was not weird for us to see the :: symbol being removed left and right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh nvm, looks like the entire function was deprecated some time ago

Copy link
Collaborator

Choose a reason for hiding this comment

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

Only the non-namespaced version is deprecated. But don't worry about a PR right now. I'm about to throw in a PoC/RFC style PR that touches this function. Would be interested to hear your thoughts.

@alexjfisher alexjfisher mentioned this pull request Feb 14, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexjfisher alexjfisher alexjfisher left review comments

@ekohl ekohl ekohl left review comments

@david22swan david22swan david22swan approved these changes

@jordanbreen28 jordanbreen28 jordanbreen28 approved these changes

@b4ldr b4ldr Awaiting requested review from b4ldr b4ldr is a code owner

@bastelfreak bastelfreak Awaiting requested review from bastelfreak bastelfreak is a code owner

@smortex smortex Awaiting requested review from smortex smortex is a code owner

Assignees
No one assigned
Labels
backwards-incompatible
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@LukasAud @alexjfisher @ekohl @david22swan @jordanbreen28 @chelnak