fix: make tedge-write not reset owner/group #2974
Merged
Conversation
This file contains hidden or 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
Robot Results
|
493a953 to
82ad08d
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
|
43f5705 to
d4bc9ce
Compare
d4bc9ce to
e05e910
Compare
61da3df to
5e0cb80
Compare
fixes: thin-edge#2972 Fixed two closely related problems in tedge-write: - When destination file existed, its owner/group was reset because when creating a temporary file for atomic write we forgot to set the same permissions as the destination file. Now we copy destination file permissions to the temporary, ensuring that after the rename permissions stay the same. - Changed some permissions used in the tests from 644 to 666, so the tests don't fail when default umask is different. Also added new functionality: tedge-write can now use plain uid/gid numbers to change owner instead of needing a name. This was done so we can more easily test file owner changes, but is probably a user improvement in its own right. Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
To properly test tedge-write, we need to be running in the environment where 1. sudo is installed 2. we run as `tedge` user 3. there is a sudoers entry that allows tedge to `sudo tedge-write` without password RobotFramework provides that environment, so tests of tedge-write functionality were moved there. Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
5e0cb80 to
6dfdbc5
Compare
|
69bdfe1 is expected to fix the failing |
69bdfe1 to
4176c35
Compare
|
After verifying with @reubenmiller, we decided that the test was indeed checking the wrong thing, so the fix was to change the test itself. I decided not to bother with the umask in Cumulocity Configuration Operation suite, as it should be already handled in the Tedge Write suite. Should be good to merge now. |
Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
Test `Set Configuration when file exists` is changed to be consistent with its comment that suggests that permissions for existing files should not change. The test now actually asserts that permissions of `/etc/config1.json` and `/etc/binary-config1.tar.gz` stay unchanged. Previously, the test seemed to actually assert if the permission did change, despite the comment claiming otherwise. Changing of permission was not desirable, and along with resetting the owner of the file, was part of a bug that is now fixed, so either the test must've been wrong to begin with, or tedge-write behaviour is specified incorrectly. `/etc/config1.json` was created with permissions 664, but the test verified that they were 644 after the operation, which isn't consistent with what's in `tedge-configuration-plugin.toml`, but is the default file permissions with default umask 002, so a new file being created without any changes to default permissions fulfilled the requirements of the test. Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
b94ea1f to
70f761f
Compare
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
Fixed two closely related problems in tedge-write:
Also added new functionality: tedge-write can now use plain uid/gid numbers to change owner instead of needing a name. This was done so we can more easily test file owner changes, but is probably a user improvement in its own right.
Types of changes
Paste Link to the issue
Checklist
cargo fmtas mentioned in CODING_GUIDELINEScargo clippyas mentioned in CODING_GUIDELINESFurther comments