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

Convert relative paths to absolute as their associated sls file is processed #61659

Merged

Conversation

dmurphy18
Copy link
Contributor

@dmurphy18 dmurphy18 commented Feb 17, 2022

What does this PR do?

Converts the relative path sls files to their equivalent absolute path when initially rendering their state.

What issues does this PR fix or reference?

Fixes: #54179

Previous Behavior

Would get conflicting ID's for the same sls file when rendering the paths for relative and absolute paths to the same file. For example:

salt/bug.bug
salt/bug/bug

are the same sls file from the perspective of the salt/bug directory, but would generate conflicting ids

New Behavior

When rendering the relative path salt/bug.bug, it is now converted to it's absolute path salt/bug/bug.
This is similar to converting all local time to UTC such that comparisons are simplified, likewise using absolute paths.

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.

@dmurphy18 dmurphy18 requested a review from a team as a code owner February 17, 2022 01:20
@dmurphy18 dmurphy18 requested review from MKLeb and removed request for a team February 17, 2022 01:20
@dmurphy18 dmurphy18 force-pushed the abs_rel_path_ids_bug_54179 branch from 7e90d28 to f75659b Compare February 17, 2022 23:18
@dmurphy18
Copy link
Contributor Author

Want deeply reviewed since state system

@dmurphy18 dmurphy18 requested a review from dwoz February 18, 2022 05:11
@dmurphy18 dmurphy18 force-pushed the abs_rel_path_ids_bug_54179 branch from f75659b to 96e1ca6 Compare February 22, 2022 17:46
@dmurphy18 dmurphy18 requested a review from s0undt3ch February 22, 2022 17:54
s0undt3ch
s0undt3ch previously approved these changes Feb 22, 2022
tests/pytests/integration/states/test_highstate_paths.py Outdated Show resolved Hide resolved
twangboy
twangboy previously approved these changes Feb 22, 2022
@dmurphy18 dmurphy18 requested a review from s0undt3ch February 28, 2022 17:01
waynew
waynew previously approved these changes Feb 28, 2022
Copy link
Contributor

@waynew waynew left a comment

Choose a reason for hiding this comment

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

Well, the only thing that I was particularly worried about was something like the IDs changing - it does change __sls__ if you're using --out=json, but aside from that, based on some internal discussion/exploration it doesn't seem to be a problem, and all of the tests are passing, especially saltcheck which looks like one of the main places that __sls__ is being used.

I don't have any reason to block this, and I've tested it locally and it appears to do what it says on the tin 👍

@dmurphy18 dmurphy18 dismissed stale reviews from waynew and twangboy via 10033ff March 1, 2022 19:02
@dmurphy18 dmurphy18 force-pushed the abs_rel_path_ids_bug_54179 branch from 03cb0ff to 10033ff Compare March 1, 2022 19:02
@dmurphy18
Copy link
Contributor Author

re-run fedora
re-run windows

@Ch3LL Ch3LL added the Phosphorus v3005.0 Release code name and version label Mar 8, 2022
@dmurphy18
Copy link
Contributor Author

re-run freebsd

Copy link
Contributor

@waynew waynew left a comment

Choose a reason for hiding this comment

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

🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Phosphorus v3005.0 Release code name and version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include issue with absolute and relative path result in conflicting IDs
6 participants