Skip to content

Deprecated Ruby 3.2 method exists? is still used #1354

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

Closed
smoeding opened this issue May 12, 2023 · 6 comments · Fixed by #1357
Closed

Deprecated Ruby 3.2 method exists? is still used #1354

smoeding opened this issue May 12, 2023 · 6 comments · Fixed by #1357

Comments

@smoeding
Copy link
Contributor

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
@pmcmaw
Copy link
Contributor

pmcmaw commented May 12, 2023

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

@LukasAud
Copy link
Contributor

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.

@LukasAud
Copy link
Contributor

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
Copy link
Collaborator

ekohl commented May 22, 2023

@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.

@LukasAud
Copy link
Contributor

LukasAud commented May 22, 2023

@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
Copy link
Collaborator

ekohl commented May 22, 2023

#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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants