Fix salt.states.file.managed() for follow_symlinks=True and test=True #62067
Conversation
|
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.
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. |
petrpavlu
force-pushed
the
file-managed-symlinks-fix
branch
from
34732da to
61f2f3a
Compare
|
Updated patch should address test failures on FreeBSD and macOS. |
petrpavlu
force-pushed
the
file-managed-symlinks-fix
branch
from
61f2f3a to
6d10245
Compare
|
Hi @petrpavlu, thanks for your contribution! I think everything looks good here. Going to let the remaining workflows run and will come back to this tomorrow to approve if all is good. |
When managing file /etc/test as follows: > file /etc/test: > file.managed: > - name: /etc/test > - source: salt://config/test > - mode: 644 > - follow_symlinks: True and with /etc/test being a symlink to a different file, an invocation of "salt-call '*' state.apply test=True" can report that the file should be updated even when a subsequent run of the same command without the test parameter makes no changes. The problem is that the test code path doesn't take correctly into account the follow_symlinks=True setting and ends up comparing permissions of the symlink instead of its target file. The patch addresses the problem by extending functions salt.modules.file.check_managed(), check_managed_changes() and check_file_meta() to have the follow_symlinks parameter which gets propagated to the salt.modules.file.stats() call and by updating salt.states.file.managed() to forward the same parameter to salt.modules.file.check_managed_changes(). Fixes saltstack#62066.
petrpavlu
force-pushed
the
file-managed-symlinks-fix
branch
from
6d10245 to
d58a3d6
Compare
|
Updated the patch to fix the reported isort failure. |
|
Thanks @MKLeb for the review. Can the patch be now merged or is still anything missing? |
|
Congratulations on your first PR being merged! 🎉 |
What does this PR do?
When managing file
/etc/testas follows:and with
/etc/testbeing a symlink to a different file, an invocation ofsalt-call '*' state.apply test=Truecan report that the file should be updated even when a subsequent run of the same command without the test parameter makes no changes.The problem is that the test code path doesn't take correctly into account the
follow_symlinks=Truesetting and ends up comparing permissions of the symlink instead of its target file.The pull request addresses the problem by extending functions
salt.modules.file.check_managed(),check_managed_changes()andcheck_file_meta()to have thefollow_symlinksparameter which gets propagated to thesalt.modules.file.stats()call and by updatingsalt.states.file.managed()to forward the same parameter tosalt.modules.file.check_managed_changes().What issues does this PR fix or reference?
Fixes #62066.
Previous Behavior
Unexpected updates were reported by
salt.states.file.managed()withfollow_symlinks=Trueand applying withtest=True, when running without the test parameter produced no changes. See #62066 for details.New Behavior
The test mode for
salt.states.file.managed()takes into accountfollow_symlinks=Trueand doesn't report the unexpected updates.Merge requirements satisfied?
[NOTICE] Bug fixes or features added to Salt require tests.
Commits signed with GPG?
No
Please review Salt's Contributing Guide for best practices.
See GitHub's page on GPG signing for more information about signing commits with GPG.