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

[KeyVault-Certificates] Last API changes for alignment with other languages #6567

Closed
sadasant opened this issue Dec 14, 2019 · 2 comments · Fixed by #6644
Closed

[KeyVault-Certificates] Last API changes for alignment with other languages #6567

sadasant opened this issue Dec 14, 2019 · 2 comments · Fixed by #6644
Assignees
Labels

Comments

@sadasant
Copy link
Member

@sadasant sadasant commented Dec 14, 2019

  • (sadasant) UpdateCertificatePolicyOptions should not extend CertificateProperties, just coreHttp.OperationOptions.
  • (sadasant) beginRecoverDeletedCertificate should return a certificate with policy.
  • (sadasant) backupCertificate returns Promise<Uint8Array | undefined>, should return Promise
  • (sadasant) Remove all the maxresults of all the list().byPage option bags.
  • (sadasant) getCertificateOperation should return a certificate with policy at the end.
  • (sadasant) setContacts returns Promise<CertificateContact[] | undefined>, return Promise<CertificateContact[]>
  • (sadasant) getContacts returns Promise<CertificateContact[] | undefined>, return Promise<CertificateContact[]>
  • (joturner) Change importCertificate's certificateValue to certificateBytes.
  • (joturner) CertificateProperties vaulUrl should be readonly. Make sure everything that should be readonly is in CertificateOperation.
  • (joturner) CertificateOperation needs name and vaultUrl.
  • (joturner) DeletedCertificate recoveryId should be readonly.
  • (joturner) LifetimeAction's Action shouldn't be optional.
  • AdministratorContact: require at least one prop.
  • (joturner) Exported types shouldn't be | undefined.
  • [sadasant] Flatten out IssuerProperties in CertificateIssuer. There shouldn’t be a properties property on CertificateIssuer, but there should be a provider property.
@jonathandturner

This comment has been minimized.

Copy link
Contributor

@jonathandturner jonathandturner commented Dec 15, 2019

Took at look at a few of these and wanted to list what I found (sorry if there were any overlaps with WIP, just getting back after vacation):

  • "CertificateOperation needs name and vaultUrl." -- guessing this is the certificateName and vaultUrl. Might be worth brainstorming how to do this, as we don't currently carry those with us for LTO (at least I don't think we do)
  • "LifetimeAction's Action shouldn't be optional." -- making action optional for LifetimeAction seems to mean we need a default value, as we can't assume we'll get one back from the service
  • "AdministratorContact: require at least one prop." -- I tried using the RequireAtLeastOne, but similar to the previous one we can't assume we're getting one of the fields back from the service

Also a bit curious why we're taking out some of the | undefined. I guess for arrays we're saying that it's worth it to take the extra constructor call to simplify the API?

@sadasant

This comment has been minimized.

Copy link
Member Author

@sadasant sadasant commented Dec 16, 2019

My answers so far:

  • we don't currently carry those with us for LTO (at least I don't think we do) if we can confirm this together, we should bring that up to the team.
  • we can't assume we're getting one of the fields back from the service I agree! Let's chat with Scott or let's post it in KeyVault.
  • | undefined this was more of a question. I wrote it imperatively by mistake. I agree with either the extra constructor call or leaving it optionally undefined if that makes more sense. This also aligns with the lack of certainty from the service's output.
@ramya-rao-a ramya-rao-a added this to the [2020] January milestone Dec 16, 2019
sadasant added a commit that referenced this issue Dec 17, 2019
…guages (#6569)

For #6567

Done so far:

UpdateCertificatePolicyOptions should not extend CertificateProperties, just coreHttp.OperationOptions.
beginRecoverDeletedCertificate should return a certificate with policy.
backupCertificate returns Promise<Uint8Array | undefined>, should return Promise Jonathan and I decided to not do this.
Remove all the maxresults of all the list().byPage option bags.
getCertificateOperation should return a certificate with policy at the end This require a longer discussion around core-lro.
Flatten out IssuerProperties in CertificateIssuer. There shouldn’t be a properties property on CertificateIssuer, but there should be a provider property.
sadasant added a commit to sadasant/azure-sdk-for-js that referenced this issue Dec 18, 2019
This PR contains the implementation of a poller with a bag of public
properties. I believe the names I picked are decent, but I'll change
them if another name is suggested.

Please let me know if the current approach is not desireable too.

This also fixes the last remaining task from Azure#6567 :
- getCertificateOperation should return a certificate with policy at
the end.

Therefore:

Fixes Azure#6567
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.