-
Notifications
You must be signed in to change notification settings - Fork 20
feature: Implement provider interface in client role #222
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a provider interface for the Client role to manage local users, groups, and sudo rules, along with a BareClientTopologyController for setting up bare client test environments. This is a valuable enhancement for testing. My review primarily focuses on the new LocalSudoRule class, where I've identified several critical issues that could lead to incorrect behavior or runtime errors. I've also included some medium-severity suggestions to improve robustness and documentation.
5f786c2 to
85f9c21
Compare
f6e7938 to
bd42ff8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a provider interface to the Client role, allowing it to manage local users, groups, and sudo rules, which is a great enhancement for testing. It also adds a BareClientTopologyController for setting up clients without a full provider. The implementation of LocalSudoRule is comprehensive but has a few issues related to rule string generation and filename uniqueness that could lead to bugs in tests. I've provided suggestions to address these correctness issues and improve documentation consistency.
d9b362d to
518f179
Compare
f610e15 to
f0a6b81
Compare
danlavu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, just nitpicks. I haven't tested it yet, but I will approve after I test it. Thank you.
8542ed7 to
16103d9
Compare
bd74c5b to
71b334d
Compare
3c78531 to
5d5daac
Compare
69a783f to
1a7c206
Compare
1a7c206 to
5945ef1
Compare
aplopez
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Allow Client to be used as provider by implementing user, group, sudorule interfaces. Implement LocalSudoRule class. Add version comparison method to BaseLinuxHost.
5945ef1 to
7258974
Compare
Allow Client to be used as provider by implementing user, group, sudorule interfaces.
Implement LocalSudoRule class.
Extend BareClient with BareClientTopologyController that setups local domain using either files-provider or proxy files.