puppetlabs/puppetlabs-stdlib

Deprecated Ruby 3.2 method exists? is still used

smoeding opened this issue · 6 comments

Describe the Bug

Ruby 3.2 (used by Puppet-Agent 8) has removed the deprecated methods {File,Dir}.exists?: https://github.com/puppetlabs/puppet/wiki/Puppet-8-Compatibility

Currently File.exists? is still used by three parser functions: loadyaml, loadjson and load_module_metadata.
Using one of these functions with Puppet 8 will probably not work as expected.

Expected Behavior

The functions should work when Puppet 8 is installed.

Environment

  • 8.0.0
  • Debian-11

Hey @smoeding this module is still currently being worked on to become Puppet 8 compatible therefore this piece of work will be addressed in #1307

Hey @smoeding, we have investigated this issue and the possibility of .exists? causing failures in our code. It seems, however, that while it is true that starting Puppet 8, we are deprecating .exists? in favor of .exist?, our Stdlib module contains its own definition of the former here. We have tested all three occurrences of .exists? in the code on Puppet 8 and they seem to be working correctly.

I believe this issue can be closed. However, if you do experience any failures related to this, feel free to reopen it with additional information about the issue.

ekohl commented

@LukasAud I think that's not quite true. The example you linked implements a Puppet API, but the real problems are here:

elsif File.exists?(args[0]) # rubocop:disable Lint/DeprecatedClassMethods : Changing to .exist? breaks the code

elsif File.exists?(args[0]) # rubocop:disable Lint/DeprecatedClassMethods : Changing to .exist? breaks the code

metadata_exists = File.exists?(metadata_json) # rubocop:disable Lint/DeprecatedClassMethods : Changing to .exist? breaks the code

They're disabled in RuboCop but I'm very certain that (as @smoeding reported) will break on Ruby 3.2. It may appear to work because in the tests the methods are mocked.

@ekohl My conclusion came as a result of the failure messages I noticed while attempting to implement changes similar to the ones in #1357. The entire logic of the classes that were being 'fixed' started to fail on me and thus, I figured out it probably had something to do with the defined API method. Perhaps my conclusion and/or approach was wrong but I assumed the code was working (despite the usage of .exists?) when I noticed test files such as loadjson_spec.rb passing green despite them requiring the overall logic to be functional.

Again, I might have taken the wrong approach so I will re-investigate this issue a bit later.

ekohl commented

#1357 should address this issue.

As for the Puppet side, https://github.com/puppetlabs/puppet/blob/ad7d75b08dfff5e308fde199407d84308d74e538/lib/puppet/property/ensure.rb#L82-L85 is the relevant bit where it changes behavior if a provider implements exist?, so exactly what you linked earlier.