Board Review: Azure Key Vault Certificates Full API Review

schaabs commented 5 years ago

Language	Review
.NET	Azure.Security.KeyVault.Certificates
Java	com.azure.security.keyvault.certificates
JS	@azure/keyvault-certificates
Python	azure-keyvault-certificates

adrianhall commented 5 years ago

adrianhall commented 5 years ago

.NET Review: (no recording available)

Bunch of changes already made since this review is not GA
Rename name to what the name is of (e.g. certificateName, issuerName)
Should we allow DeleteIssuer to take an issuer? We are doing symmetrical things elsewhere.
- Leave as is
GetCertificates rename to GetPropertiesofCertificates
- Convert includePending to bool = false
GetIssuers rename to GetPropertiesOfIssuers
RestoreCertificate rename to RestoreCertificateBackup
CertificateImportOptions rename to ImportCertificateOptions (ditto other options types)
Review separate clients offline.
Remove DefaultPolicy
Make Value and Policy read-only on ImportCertificateOptions
Should AdministratorDetails and Contact be the same type?
- Leave separate, but rename AdministratorDetails to AdministratorContact
Id vs. Uri - we are compatible with other libraries, but not with other languages.
- Given that it is not treated as a Uri, it should be a string. It's only a .NET change.
- Peter is supportive of the change.
- Decision: Change KeyId / SecretId to strings across the KeyVault board.
vaultEndpoint - we should rename it to vaultUrl due to mgmt consistency (all languages)
In CertificateWithPolicy, rename Policy to IssuancePolicy or similar.
Rename Self and Unknown to better identify them - take offline.

@KrzysztofCwalina to add guidelines for .NET on nullable booleans. @KrzysztofCwalina to add guidelines around specifying time spans (in months, etc.)

adrianhall commented 5 years ago

@bterlson will re-review once final API is uploaded.

Rename CertificateIssuer to CertificateIssuerProperties (follow up offline)
Rename expires to expiresOn
Consider renaming id on CertificateProperties to certificateId - check consistency
Rename getCertificate to getCertificateVersion
getCertificateOperation should return a poller
Rename getCertificateWithPolicy to getCertificate
Remove getDefaultPipeline
Rename listCertificates to listPropertiesOfCertificate (and similar for versions)
Remove the pipeline public property
Rename Contact to either KeyVaultContact or CertificateContact - design offline
Rename ErrorModel to CertificateOperationError
Rename innerError property to details
Rename updated to updatedOn and created to createdOn
Rename IssuerAttributes to IssuerProperties
Ensure generated code types are not exported unless necessary, and renamed as needed
UpdateCertificatePropertiesOptions should be flattened and only expose settable properties
Action and Trigger need to be exported

adrianhall commented 5 years ago

In createCertificate, rename enabled to isEnabled
Remove overload of getCertificates(CertificateProperties) for now (post-GA) - decide offline
Remove mergeCertificate(string name,...) as a convenience method right now (post-GA)
Rename listIssuers to listPropertiesOfIssuers
Rename restoreCertificate to restoreCertificateBackup
Change endpoint to vaultUrl
Rename Administrator to AdministratorContact
Remove setCertificatePolicy() on Certificate
Use on suffix for date properties.
Rename Contact to CertificateContact
Rename Issuer to CertificateIssuer

Azure / azure-sdk