-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Should file.rename return success when target exists and force is not used? #49748
Comments
Addendum: file.copy behaves as expected, an already existing version is not a failure but returns success: So, using file.copy is a proper workaround, but still file.rename should behave consistently.
|
So, without |
im not able to replicate this on 2018.3.2 with this state:
on the second run i see:
When you pasted your salt version it looks like that is the version on your master. What is your minion version? |
As shown in the top commandline of my test case: I used the master's own '$HOSTNAME', so basically that was a localhost test. Edit: What I wanted to say: Master Version == Minion Version, since that's been the same host. Your test case is incomplete though. You're only moving the file out of the way, but if you replace the initial filename with a new one in a following file.managed state, that should trigger the issue. See the file.managed state in my sls on top. |
thanks for clarifying that. I used your exact state and can now replicate it and I agree this should be reporting success. will label as a bug thanks :) as @MTecknology stated this should be an easy fix if you want to give it a go |
Sure, just wanted to make sure in advance, that my expectation was indeed correct. I'll take a look. |
I'm reading your "Contributing" documentation right now, but that fix is so trivial, that a git pull request feels massively overkill for me, so here's a context diff. I think the return ret['comment'] is fine, so the only thing that's changed is the removal of the False return value (ret['result'] is initialized on top of the function with True).
|
Mmm this is a behavior change. Someone might rely on the fact it errors. Another option is to add a |
Yes, this is a behavior change, but this discussion shows that the previous behavior was unexpected. The use case presented here seems substantially more likely than a use case that expects a failure in this situation. This change makes the behavior between file.copy:
file.rename:
|
Description of Issue/Question
Use-case:
Works correct for the first run, when package-x.conf.example doesn't exist yet.
Fails on consecutive runs, because package-x.conf.example already exists.
Using 'force=True' is not an option, because after the second state.apply, the packaged version would be lost.
Shouldn't the file.rename report a "success, renamed file already in correct state" instead of a failure when force is not used?
Misunderstanding? Or am I doing something non-idiomatic here and there's a better way?
Setup
Steps to Reproduce Issue
Initial run
next run
Versions Report
The text was updated successfully, but these errors were encountered: