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

Fix/wheel file roots find #59803

Closed

Conversation

kevthehermit
Copy link

What does this PR do?

Adds subdir=True to wheel.file_roots.find

What issues does this PR fix or reference?

Fixes: #59800

Previous Behavior

When attempting to read a file in a subdirectory with wheel.file_roots.read it would fail as wheel.file_roots.find is not able to read from a subdirectory.

New Behavior

You can successfully read a file in a subdirectory

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

@kevthehermit kevthehermit requested a review from a team as a code owner March 14, 2021 15:18
@kevthehermit kevthehermit requested review from Ch3LL and removed request for a team March 14, 2021 15:18
@welcome
Copy link

welcome bot commented Mar 14, 2021

Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here’s some information that may help as you continue your Salt journey.
Please be sure to review our Code of Conduct. Also, check out some of our community resources including:

There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar.
If you have additional questions, email us at core@saltstack.com. We’re glad you’ve joined our community and look forward to doing awesome things with you!

Copy link
Contributor

@Ch3LL Ch3LL left a comment

Choose a reason for hiding this comment

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

This will require test coverage

@Ch3LL
Copy link
Contributor

Ch3LL commented Mar 16, 2021

@dwoz would you mind reviewing here. Looks like this was changed here: d5801df

@Ch3LL Ch3LL requested a review from dwoz March 16, 2021 11:30
Copy link
Contributor

@dwoz dwoz left a comment

Choose a reason for hiding this comment

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

While looking at the implementation of subdir=True, I am a bit confused

    if subdir:
        if real_path.startswith(real_root):
            return real_path
    else:
        if os.path.dirname(real_path) == os.path.normpath(real_root):
            return real_path

Why are we calling normpath in the else and not the subdir case?

@dwoz
Copy link
Contributor

dwoz commented Mar 17, 2021

@dwoz would you mind reviewing here. Looks like this was changed here: d5801df

I approve provided we have tests.

@kevthehermit
Copy link
Author

I didn't see any existing test in place for these modules to extend so will have to read up on tests and put something together

@sagetherage sagetherage added the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Apr 30, 2021
@sagetherage
Copy link
Contributor

@waynew maybe a good candidate for the Test Clinic next Tuesday

@MKLeb
Copy link
Contributor

MKLeb commented Jun 6, 2023

This will close the corresponding issue with a comprehensive test suite: #64429.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

salt.wheel.file_roots.find missing subdir=true
5 participants