-
Notifications
You must be signed in to change notification settings - Fork 71
Add ListClients and DeleteClient operations #318
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
@@ -14,6 +14,7 @@ socket_path = "/tmp/parsec.sock" | |||
|
|||
[authenticator] | |||
auth_type = "Direct" | |||
admins = [ { name = "list_clients test" }, { name = "parsec-client-1" }, { name = "client1" }, { name = "spiffe://example.org/parsec-client-1" } ] |
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.
Having a list of admin names is definitely useful 👌
let _app_name = | ||
unwrap_or_else_return!(app_name.ok_or(ResponseStatus::NotAuthenticated)); |
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 not needed I think?
11e1c10
to
0c5159e
Compare
@@ -330,7 +330,7 @@ impl ServiceChecker { | |||
.expect("Verification failed"); | |||
|
|||
client | |||
.destroy_key(sign_key_name.clone()) | |||
.destroy_key(sign_key_name) |
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.
We've not added clippy to our e2e_tests, maybe we should do that
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.
👍
Also adds multitenancy tests for those operations and for the admin feature. Splits the ApplicationName structure into Application and ApplicationName to only deal with key names in the Key Info Managers and not the admin status which is not relevent there. Signed-off-by: Hugues de Valon <[email protected]>
As in, shall we merge this for now and apply what we discuss on the meeting on Parsec and the documentation afterwards? Or shall we wait? |
Merge this one, we can apply the changes afterwards in a separate PR |
Also adds multitenancy tests for those operations and for the admin
feature. Splits the ApplicationName structure into Application and
ApplicationName to only deal with key names in the Key Info Managers and
not the admin status which is not relevent there.
Fix #309
Fix #311