Skip to content

Remove deprecated File.exists? #1357

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

Merged
merged 2 commits into from
May 25, 2023
Merged

Conversation

ekohl
Copy link
Collaborator

@ekohl ekohl commented May 22, 2023

In 7999ff2 the cops were disabled, but Ruby 3.2 has removed the method.

Fixes #1354

@ekohl ekohl requested review from a team, alexjfisher, b4ldr, bastelfreak and smortex as code owners May 22, 2023 09:59
@puppet-community-rangefinder
Copy link

load_module_metadata is a function

Breaking changes to this file MAY impact these 10 modules (near match):

loadjson is a function

Breaking changes to this file MAY impact these 1 modules (near match):

loadyaml is a function

Breaking changes to this file MAY impact these 2 modules (near match):

This module is declared in 318 of 580 indexed public Puppetfiles.


These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

bastelfreak
bastelfreak previously approved these changes May 22, 2023
@LukasAud
Copy link
Contributor

Hey @ekohl, it looks like the PR is generating a block of failures in the CI. I dont think we can merge this while in this state. Also, we are planning to merge the Puppet 8 update to main very soon (today/tomorrow morning, you are welcome to leave a review if you want), so this PR will need a rebase to ensure that CI covers testing properly.

I will try to investigate this issue again as well, to see if I can figure out whats going on or why CI is not failing on main despite .exists? being present there.

@ekohl
Copy link
Collaborator Author

ekohl commented May 23, 2023

That looks very suspicious, but I can't explain why it happens. I don't have time to investigate it now so feel free to take over if you have the cycles.

@LukasAud
Copy link
Contributor

@ekohl Sure thing, I'll rebase the PR and play around with for a bit to see if I can figure out a reason for these failures.

@CLAassistant
Copy link

CLAassistant commented May 24, 2023

CLA assistant check
All committers have signed the CLA.

In 7999ff2 the cops were disabled, but
Ruby 3.2 has removed the method.
@ekohl
Copy link
Collaborator Author

ekohl commented May 24, 2023

I rebased it myself because I suspect the merge commit tripped up the CLA check.

File#exist? is called in many locations.  If we want to mock its behavior
for some use cases, we must ensure that only the specific test case is
mocked and the other calls still rely on the original implementation.
@smortex
Copy link
Collaborator

smortex commented May 24, 2023

The mocked tests seems to "hide" the original code. Allowing all calls to File#exist? and calling the original seems to fix the issue.

@LukasAud
Copy link
Contributor

I was looking into this yesterday. Looks like I was getting close to the solution but I couldnt quite figure out exactly how to fix it. Anyways, happy to merge this now.

Copy link
Contributor

@LukasAud LukasAud left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @ekohl and @smortex for your contributions.

@LukasAud LukasAud merged commit 2c61c1b into puppetlabs:main May 25, 2023
@ekohl ekohl deleted the remove-exists branch May 25, 2023 09:10
@ekohl
Copy link
Collaborator Author

ekohl commented May 25, 2023

The mocked tests seems to "hide" the original code. Allowing all calls to File#exist? and calling the original seems to fix the issue.

That was also one of my suspicions, but was too busy with other work. Thanks @smortex!

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.

Deprecated Ruby 3.2 method exists? is still used
6 participants