-
Notifications
You must be signed in to change notification settings - Fork 299
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
Update credentials for more valid values. #13
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.
Changes LGTM
There is additional testing effort, to cover old scenarios that are broken due to a bad interpretation of code by my side.
Also there are new unit tests scenarios to cover.
@@ -8,9 +9,15 @@ public class UserCredentials | |||
[YamlMember(Alias = "client-certificate-data")] | |||
public string ClientCertificateData { get; set; } | |||
|
|||
[YamlMember(Alias = "client-certificate")] | |||
public string ClientCertificate { get; set; } |
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.
Need to review unit tests. Thought that client-certifidate-data hold the route to a X509 certificate, but concluded now it is the certificate itself. Additional tests should be written to cover the client-certificate scenario.
[YamlMember(Alias = "client-key-data")] | ||
public string ClientKeyData { get; set; } | ||
|
||
[YamlMember(Alias = "client-key")] | ||
public string ClientKey { get; set; } |
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.
Same as client-certificate. Thought client-key-data stored a path, but they are the certificate itself. Now several tests are broken.
1af9978
to
f1f575e
Compare
@sesispla I fixed the tests. Would you like me to add new tests to this PR or as part of a later commit? |
@brendanburns Maybe we can add some basic tests for client-certificate-data, and then create a more intensive test plan in the future. Not perfect, but it will the job for now. For the future plan, I am looking for a Code Coverage tool in #16 which will make our life easier. |
f732918
to
c42d56c
Compare
588979c
to
bad9df9
Compare
@sesispla sorry for the delay. I added some tests, and then finally got them passing :) Please take another look. Thanks! |
@brendanburns can't see any new commits to this PR. Maybe they are in another branch? Or are they 6333784? |
6333784 LGTM |
@sesispla yeah, I squashed them down to one commit.... Thanks for the reivew! |
@sesispla @krabhishek8260
This makes it possible to use csharp with the
minikube
tool.