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

Updated the salt.states.file.copy function to make the call to file.remove with the force parameter set to the same as one for salt.states.file.copy. #51740

Merged
merged 5 commits into from
Mar 7, 2019

Conversation

arizvisa
Copy link
Contributor

What does this PR do?

On the windows platform, the salt.state.file.copy function does not handle files with the read-only attribute properly. When force is set to true, the file.copyfunction usesfile.removeto attempt to remove the target before copying it. This function fails with an exception when the read-only attribute is set. However whenfile.removehas itsforce` parameter set to true, this gets handled properly.

This PR passes the value of the force parameter from file.copy to its call to file.remove so that files with the read-only attribute are handled.

What issues does this PR fix or reference?

This closes issue #51739.

Previous Behavior

Previously when file.copy tried to remove the target directory it would fail when encountering a read-only file.

New Behavior

Now when force is set to true, the call to file.remove will inherit the same value which allows file.copy to successfully remove the target directory.

Tests written?

No

Commits signed with GPG?

No

…emove with the force parameter set to the same as one for salt.states.file.copy.
arizvisa added a commit to arizvisa/lolfuzz3 that referenced this pull request Feb 21, 2019
…allow file.copy to work on windows with read-only files.
@arizvisa
Copy link
Contributor Author

arizvisa commented Feb 21, 2019

This PR seems to conflict with issue #25250. I'll have to look at it the next time I get free time, but in summary it looks like file.copy used to not check the differences between files before transferring them.

This fix is essentially a 12-byte patch that only passes the force parameter to file.remove, and so I'm assuming that the condition that calls file.remove is not actually checking if the file was different or not before calling it, (or the test for #25250 itself is weird which I'd imagine is unlikely).

salt/states/file.py Show resolved Hide resolved
arizvisa added a commit to arizvisa/lolfuzz3 that referenced this pull request Feb 21, 2019
…allow file.copy to work on windows with read-only files.
@arizvisa
Copy link
Contributor Author

Ok, I added the kwargs support to salt.modules.file.remove. This actually fixes a number of issues in a couple of salt.states.file functions that exist due to there being no force parameter. So good call.

arizvisa added a commit to arizvisa/lolfuzz3 that referenced this pull request Feb 23, 2019
@arizvisa
Copy link
Contributor Author

@twangboy, when you get a chance next week, lmk if there's anything else I need to do to get these merged upstream as next weekend I won't be able to get back to these saltstack PRs due to them not being even closely related to my regular job.

Thanks for your help!

@arizvisa
Copy link
Contributor Author

Hmm..why is salt.utils.is_running not being loaded in the tests for this? I walked through the backtrace and nothing there is importing my module, and I tested via the following (which is what the backtrace is doing) and it works without a problem?

$ salt --async \* state.apply
Executed command with job ID: 20190225210028096476
$ salt \* saltutil.is_running 'state.*'
888e0dab3011409888f36dbf6153f9cf:
    |_
      ----------
      arg:
      fun:
          state.apply
      jid:
          20190225210028096476
      master_id:
          888e0dab3011409888f36dbf6153f9cf
      pid:
          508
      ret:
          etcd
      tgt:
          *
      tgt_type:
          glob
      user:
          root
bX_nxPEVRyWOcIDBECwW:
    |_
      ----------
      arg:
      fun:
          state.apply
      jid:
          20190225210028096476
      master_id:
          888e0dab3011409888f36dbf6153f9cf
      pid:
          2136
      ret:
          etcd
      tgt:
          *
      tgt_type:
          glob
      user:
          root


-------------------------------------------
Summary
-------------------------------------------
# of minions targeted: 2
# of minions returned: 2
# of minions that did not return: 0
# of minions with errors: 0
-------------------------------------------
$

@dwoz dwoz merged commit 19a1882 into saltstack:develop Mar 7, 2019
@arizvisa
Copy link
Contributor Author

arizvisa commented Mar 7, 2019

Thanks!

garethgreenaway added a commit to garethgreenaway/salt that referenced this pull request Sep 19, 2019
garethgreenaway added a commit to garethgreenaway/salt that referenced this pull request Sep 30, 2022
Ch3LL pushed a commit that referenced this pull request Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants