Extend basic test cases to check shadow and gshadow entries #1237
Conversation
|
This PR is blocked by https://gitlab.alpinelinux.org/alpine/aports/-/issues/16979. Apparently Alpine's getent doesn't provide |
|
@jubalh do you know why opensuse builder is failing? It seems a dependency installation problem: |
ikerexxe
force-pushed
the
test-useradd
branch
2 times, most recently
from
1c0a78a to
2dfbdec
Compare
danlavu
left a comment
danlavu
left a comment
There was a problem hiding this comment.
I'd like it a lot more if it followed the structure of the other roles.
shadow.user.add()
shadow.user.get()
shadow.user.modify()
shadow.group.add()
shadow.group.get()
shadow.group.modify()
tools.getent_shadow()
So we can abstract the class?
We have decided to follow an approach as similar as possible to the CLI in shadow. So, instead of useradd or groupadd the framework should provide shadow.useradd() or shadow.groupadd(). If you want a little more context I recommend you read #835 (comment).
The behaviour with getent is the same one as in SSSD. What you mention there would be different. |
Ack, I will read and continue the review then. Thank you for the clarification. |
danlavu
left a comment
danlavu
left a comment
There was a problem hiding this comment.
Everything looks great, there is only one thing I do not like, and it's mostly a personal preference. Just the number of test files, I find it easier to work on in one file than having to open several files to view the tests. My only suggestion is to create one test file test_shadow.py and have test case names like test_shadow__groupadd, etc.
Going to approve it, kindly take the single test file into consideration. Having a file for one test seems strange to me.
|
Currently there is only one test case per file because so far I have only had time to work on the PoC framework. As such, I focused on providing the most basic implementation and some examples to show how the framework works. Once the transformation from the old framework is complete I anticipate having over 1000 test cases in total, and about 100 per file. |
|
That makes perfect sense, disregard my comment then. |
Provide a way for the system framework to run `getent shadow $name` and check its output in a meaningful way. Signed-off-by: Iker Pedrosa <[email protected]> Reviewed-by: Dan Lavu <[email protected]>
Provide a way for the system framework to run `getent gshadow $name` and check its output in a meaningful way. Signed-off-by: Iker Pedrosa <[email protected]> Reviewed-by: Dan Lavu <[email protected]>
Implement a general function to detect features in shadow host. Apparently, musl doesn't provide `getent gshadow`, but shadow still needs it to check for several group attributes. Thus, check whether it exists in the host, and if it does run it. If not, let's just skip that part of the test. Link: <https://gitlab.alpinelinux.org/alpine/aports/-/issues/16979> Signed-off-by: Iker Pedrosa <[email protected]> Reviewed-by: Dan Lavu <[email protected]>
Signed-off-by: Iker Pedrosa <[email protected]> Reviewed-by: Dan Lavu <[email protected]>
Signed-off-by: Iker Pedrosa <[email protected]> Reviewed-by: Dan Lavu <[email protected]>
openSUSE includes comment lines in `/etc/os-release` file and this can cause some issues during the distribution detection. Ignore those lines as they don't cause any effect on the system. Signed-off-by: Iker Pedrosa <[email protected]> Reviewed-by: Dan Lavu <[email protected]>
Alpine Linux versions also contain the revision, and this needs to be taken into account when detecting it. Signed-off-by: Iker Pedrosa <[email protected]> Reviewed-by: Dan Lavu <[email protected]>
The test framework PoC only provided basic checks. I've added additional functionality to the framework by checking shadow and gshadow entries and I've extended the basic useradd test to check those too. Signed-off-by: Iker Pedrosa <[email protected]> Reviewed-by: Dan Lavu <[email protected]>
Add additional checks for shadow and gshadow entries. Signed-off-by: Iker Pedrosa <[email protected]> Reviewed-by: Dan Lavu <[email protected]>
Add additional checks for shadow and gshadow entries. Signed-off-by: Iker Pedrosa <[email protected]> Reviewed-by: Dan Lavu <[email protected]>
Add additional check for gshadow entry. Signed-off-by: Iker Pedrosa <[email protected]> Reviewed-by: Dan Lavu <[email protected]>
Add additional check for gshadow entry. Signed-off-by: Iker Pedrosa <[email protected]> Reviewed-by: Dan Lavu <[email protected]>
Add additional check for gshadow entry. Signed-off-by: Iker Pedrosa <[email protected]> Reviewed-by: Dan Lavu <[email protected]>
|
I'd like to proceed merging this PR but I need at least one approval (Dan only has read-only permissions and apparently doesn't count). @hallyn @alejandro-colomar can any of you approve the PR? By the way, Serge, would you mind increasing Dan's permissions so that his reviews count? He's a colleague of mine and has a great track record in testing. |
There was a problem hiding this comment.
Thanks!
Disclaimer: I have not reviewed this, but trust you if you think it's okay. If you want more review, maybe wait for @hallyn .
|
No, I think Dan's review is enough so let's merge it. Thank you for "approving" 😉 |
3 participants
The test framework PoC only provided basic checks. I've added additional
functionality to the framework by checking shadow and gshadow entries
and I've extended the basic test cases to check those too.
This should give us more coverage over the changes we make to the basic
functionality and ensure that we don't change anything inadvertently.