Fix rename #1914
Merged
Fix rename #1914
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Ruadhri17
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
Robot Results
Passed Tests
|
Ruadhri17
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
Ruadhri17
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
Ruadhri17
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
Ruadhri17
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
Comment on lines
87
to
84
| pub fn new(target_path: &Path, target_permission: PermissionEntry) -> Self { | ||
| pub fn new(target_path: &Path) -> Self { |
There was a problem hiding this comment.
I was surprised to that parameter removed. But you are correct, this can be safely removed as never assigned other than a default value.
There was a problem hiding this comment.
Hmm in c8y-configuration-plugin, there is parameters user, group and mode, where user can decide which permission they want to have in a downloaded file. I feel this change is breaking it.
In crates/extensions/c8y_http_proxy/src/actor.rs, this one is used. So, you cannot remove safely.
let downloader: Downloader = Downloader::new(&request.file_path);
Comment on lines
116
to
118
| // Copy source to destination including permission bits and remove source after successful copy operation | ||
| tokio::fs::copy(src_path, dest_path) | ||
| .and_then(|_| tokio::fs::remove_file(src_path)) | ||
| .await?; | ||
|
|
||
| tokio::fs::rename(src_path, dest_path).await?; |
There was a problem hiding this comment.
I would not directly copy/delete the file but try first to rename it as the copy/delete will not work if the file is larger than the free space (this is not unlikely with a firmware download on a resource-constraint device).
If would be better to fallback to a copy/delete only if the rename fails because of a cross-filesystem rename, but unfortunately this is a nightly feature. So just try to rename and copy/delete in case of some error.
rina23q
reviewed
Apr 18, 2023
Comment on lines
87
to
84
| pub fn new(target_path: &Path, target_permission: PermissionEntry) -> Self { | ||
| pub fn new(target_path: &Path) -> Self { |
There was a problem hiding this comment.
Hmm in c8y-configuration-plugin, there is parameters user, group and mode, where user can decide which permission they want to have in a downloaded file. I feel this change is breaking it.
In crates/extensions/c8y_http_proxy/src/actor.rs, this one is used. So, you cannot remove safely.
let downloader: Downloader = Downloader::new(&request.file_path);
rina23q
previously requested changes
Apr 18, 2023
As these permission settings are used in a single place, the simpler it's to keep the signature of the |
Ruadhri17
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
Ruadhri17
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
Oops, I just reverted changes regarding |
This must be fixed in that PR. But don't revert your revert: let @rina23q have a look first. I will do the same. |
Comment on lines
131
to
143
| let file_permissions = if let Some(mode) = original_permission_mode { | ||
| // Use the same file permission as the original one | ||
| PermissionEntry::new(None, None, Some(mode)) | ||
| } else { | ||
| // Set the user, group, and mode as given for a new file | ||
| new_file_permissions | ||
| }; | ||
|
|
||
| file_permissions.apply(dest_path)?; | ||
| debug!( | ||
| "Applied permissions: {:?} to {:?}", | ||
| file_permissions, dest_path | ||
| ); |
There was a problem hiding this comment.
This must also be done in the copy-and-delete case.
rina23q
reviewed
Apr 20, 2023
Comment on lines
131
to
143
| let file_permissions = if let Some(mode) = original_permission_mode { | ||
| // Use the same file permission as the original one | ||
| PermissionEntry::new(None, None, Some(mode)) | ||
| } else { | ||
| // Set the user, group, and mode as given for a new file | ||
| new_file_permissions | ||
| }; | ||
|
|
||
| file_permissions.apply(dest_path)?; | ||
| debug!( | ||
| "Applied permissions: {:?} to {:?}", | ||
| file_permissions, dest_path | ||
| ); |
rina23q
dismissed
their stale review
No breaking regression, therefore, stale this.
|
Note: if we had #1875, the permission issue would have been tested by pipeline... |
Ruadhri17
force-pushed
the
fix-rename
branch
2 times, most recently
from
c956efd to
bb2dcb2
Compare
Ruadhri17
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
Signed-off-by: Krzysztof Piotrowski <Krzysztof.Piotrowski@inetum.com>
Signed-off-by: Krzysztof Piotrowski <Krzysztof.Piotrowski@inetum.com>
Ruadhri17
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed changes
Replace rename with copy and delete where necessary. Calculate tmp path inside atomically write functions. More in #1899
Types of changes
Paste Link to the issue
#1899
Checklist
cargo fmtas mentioned in CODING_GUIDELINEScargo clippyas mentioned in CODING_GUIDELINESFurther comments