search

Venafi / vcert

Go client SDK and command line utility designed to simplify integrations by automating key generation and certificate enrollment using Venafi machine identity services.
https://support.venafi.com/hc/en-us/articles/217991528
Apache License 2.0
88 stars 64 forks source link

Contact Emails: Add unit tests for `getIdentity` #420

Closed maelvls closed 7 months ago

maelvls commented 7 months ago

This PR should be merged before continuing reviews on #418.

I paused working on #418 so that I could write some unit tests around getIdentity with the existing implementation (the one in the master branch). Once we have the unit tests merged to master, we can resume working on #418.

I could have added all of these tests directly to #418, but we wouldn't be sure whether the old implementation and the new implementation pass the same tests.

This way, we can decrease the chance of introducing a regression in getIdentity.

To know more about the tricks that getIdentity does, you can read https://github.com/Venafi/vcert/pull/221#issuecomment-1911747389.

maelvls commented 7 months ago

This is ready to be reviewed.

maelvls commented 7 months ago

@marcos-albornoz Could you take a look? I think I need two reviews to get this merged. Thanks!

marcos-albornoz commented 7 months ago

@marcos-albornoz Could you take a look? I think I need two reviews to get this merged. Thanks!

I've reviewed it but it's seeming this code is out of date compared with the code in PR 418 because this code is still referencing to the policy.IdentityEntry given that struct was moved to tpp package. Also I see in the PR 418 there is a more extended version of the test in this PR

maelvls commented 7 months ago

This PR only focuses on adding the missing unit tests around getIdentity without changing anything else in VCert. That's why IdentityEntry is still in the policy folder.

You are correct, I have added more test cases in https://github.com/Venafi/vcert/pull/418 to account for the new behavior in getIdentity.

Does it make sense? I am trying to add unit tests before changing everything to lower the chance of mistakenly breaking something.

maelvls commented 7 months ago

Marcos discovered a regression I introduced in #420: when foo is searched and the only result is foobar, getIdentity would mistakenly return foobar instead of returning an error.

I have added a test case to cover this. Please take another look @marcos-albornoz

maelvls commented 7 months ago

I'll rebase to pass the requirement "The base branch requires all commits to be signed."

maelvls commented 7 months ago

Success!

https://jenkins.eng.venafi.com/job/VCert/job/vcert/3176/

Screenshot 2024-01-30 at 19 03 07