internal/authz/azuredevops: Implement ValidateConnection#48465
Conversation
sashaostrikov
left a comment
There was a problem hiding this comment.
You have a nil-pointer dereference probability in your code which needs to be fixed :)
We should add tests which would have caught it, shouldn't we?
There was a problem hiding this comment.
nil-pointer dereference alert :)
when err above is not nil, then the client is nil
There was a problem hiding this comment.
That's a great catch - especially because my default test would not have caught that. Thank you. ❤️
I'm going to add a test case specifically to catch this as well.
There was a problem hiding this comment.
Hmm, seems like it's not that easy to test that without adding more if-this-is-a-test logic in the NewClient method. Let me know if the current tests look good or if I should do this anyway.
There was a problem hiding this comment.
yep, I can see that the only way to provoke an error is to break the URL. I think in this case we're fine :)
There was a problem hiding this comment.
| allErrors = append(allErrors, fmt.Sprintf("%s:%s", conn.URN, err.Error())) | |
| allErrors = append(allErrors, fmt.Sprintf("%s:%s", conn.URN, err.Error())) | |
| continue |
70cf7b3 to
26f8c1c
Compare
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff 5c25d9aeed959068c6cfb6edc9c2429bc2a44f72...26f8c1c63d7887cb4df9282414723c3c7d510f57.
|
26f8c1c to
77d8178
Compare
|
Rebased off main after merging #48209. Had to force pushed. Nothing changed since the last review. |
Follow up on #48209. Part of #47165.
Test plan