youki icon indicating copy to clipboard operation
youki copied to clipboard

Add linux_devices test

Open omprakaash opened this issue 1 year ago • 10 comments

Adds the linux_devices integration test.

omprakaash avatar Feb 26 '24 16:02 omprakaash

Codecov Report

Merging #2708 (b8bf9a1) into main (cfa2ea9) will not change coverage. Report is 2 commits behind head on main. The diff coverage is n/a.

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           

codecov-commenter avatar Feb 27 '24 02:02 codecov-commenter

Hey, due to a bug in our CI, currently the tests are not validated on runc. May I ask you to wait for https://github.com/containers/youki/pull/2707 to get merged and rebase on it, so we know that the added tests also work with runc? Thanks :)

YJDoc2 avatar Feb 27 '24 05:02 YJDoc2

It is merged now, So can you rebase and push?

YJDoc2 avatar Feb 27 '24 07:02 YJDoc2

Thank you. Should be rebased now.

omprakaash avatar Feb 28 '24 04:02 omprakaash

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).

omprakaash avatar Mar 21 '24 03:03 omprakaash

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.

YJDoc2 avatar Mar 26 '24 12:03 YJDoc2

@omprakaash ping!

YJDoc2 avatar Apr 01 '24 06:04 YJDoc2

Hey sorry for the delay. Will get back to this tomorrow.

omprakaash avatar Apr 02 '24 06:04 omprakaash

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.

omprakaash avatar Apr 04 '24 04:04 omprakaash

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.

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 :+1:

YJDoc2 avatar Apr 05 '24 06:04 YJDoc2

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.

YJDoc2 avatar May 06 '24 06:05 YJDoc2