Add linux_devices test #2708
Add linux_devices test #2708
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #2708 +/- ##
=======================================
Coverage 65.41% 65.41%
=======================================
Files 133 133
Lines 16942 16942
=======================================
Hits 11082 11082
Misses 5860 5860 |
omprakaash
force-pushed
the
linux-devices
branch
from
dce1122
to
23ec2c3
Compare
|
Hey, due to a bug in our CI, currently the tests are not validated on runc. May I ask you to wait for #2707 to get merged and rebase on it, so we know that the added tests also work with runc? Thanks :) |
|
It is merged now, So can you rebase and push? |
omprakaash
force-pushed
the
linux-devices
branch
from
23ec2c3
to
b8bf9a1
Compare
|
Thank you. Should be rebased now. |
There was a problem hiding this comment.
Hey I have a general question regarding this. I think you have copied over the impl from the original go test, which is ok ; but can we do better?
My point here is that if we are only creating three specific devices, then in the test we should only validate those three for the expected values. In the original validation function, we also consider other devices such as tty etc, but I feel we shouldn't...
Can you check if in the original go code, the validation fn is being used for any other test, and if not, can we trim it down here to just check the three devices we control?
Thanks!
|
Hello, yes this is a straightforward port from the test in oci-runtime tools and there is another test validateDefaultDevices that seems to use this function as well (in oci-runtime tools). |
Ok, so do you think we should separate both of these, and have smaller, maybe more modular functions that do tests specific to their test cases? Or this is the best solution, and breaking up function would be worse code than this? This does not have to be done in this PR, just want to know your thoughts. |
|
@omprakaash ping! |
|
Hey sorry for the delay. Will get back to this tomorrow. |
|
Hey I believe that this is fine, since these are used only for testing and the other test when added, could just call this function. Having separate functions for each type seems unnecessary to me since a lot of the logic seems to overlap for multiple device types, but happy to change this if you strongly feel otherwise. |
| let actual_permissions = file_data.permissions().mode() & 0o777; | ||
| let expected_permissions = device.file_mode(); | ||
| if expected_permissions.is_none() { | ||
| return; | ||
| } |
There was a problem hiding this comment.
We should probably get the actual permissions after checking if expected permissions are set
Fair enough. I don't have any better idea either, so if in future this proves to be an issue, we can fix ti then. Let's keep it as is now 👍 |
omprakaash
force-pushed
the
linux-devices
branch
2 times, most recently
from
ae433cf
to
6f7eaec
Compare
Signed-off-by: omprakaash <omsuseela@gmail.com> Signed-off-by: om prakaash <omsuseela@gmail.com>
omprakaash
force-pushed
the
linux-devices
branch
from
6f7eaec
to
51c38e2
Compare
Signed-off-by: Yashodhan Joshi <yjdoc2@gmail.com>
|
Hey I mad a minor change where we don't return immediately from check function if permissions are not configured, it is similar to how uid/gid validation is done in our implementation. |
There was a problem hiding this comment.
lgtm!
Thanks @omprakaash for your contribution and following up and doing the requested changes :)
Adds the linux_devices integration test.