Skip to content
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

Add CertificateType attribute to CertificateRequest #118

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

heoehmke
Copy link

When requesting a new certificate, the Venafi TPP API allows to specify a CertificateType attribute in the request body. This PR adds the optional attribute to the request as it was missing so far. We would be happy if the changes could be released as soon as possible, thank you!

@marcos-albornoz
Copy link
Contributor

Hi @heoehmke, thanks a lot for your contribution. The code looks good, only we need to update it a little bit to cover some code conventions like the related to naming enums... If you are agree I will clone your repo and then implement these little updates.
After that we need to run our build process(which contains a suite of tests) to determine that all is working as it's being expected and if all is Ok, then we can build the release.

@heoehmke
Copy link
Author

Hi @marcos-albornoz, thank you for your feedback. After testing the feature against the Venafi API, we noticed that the expected attribute value of CertificateType is not e.g. User but User: X.509 User Certificate, so the current state of the PR would not work which is why I set it to draft. Is this intended by Venafi to not just pass User or Server as values? Or is it a bug and the expected values could change in the future? Then I would suggest to not use an enum type but just a String value to have backwards compability. If it's intended and won't change I could adapt the enum values to the expected values and then of course you can clone the repo and do the modifications.

@heoehmke
Copy link
Author

heoehmke commented Feb 3, 2023

Hi @marcos-albornoz, I updated the PR to match the values expected by the Venafi API, now it would work. Could you review it and do the mentioned changes for code conventions or guide me how you expect the code? Thank you!

@heoehmke heoehmke marked this pull request as ready for review February 13, 2023 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants