Skip to content

Certs track 2 API design#5601

Merged
samvaity merged 9 commits intoAzure:masterfrom
samvaity:certs-track-2
Jun 10, 2019
Merged

Certs track 2 API design#5601
samvaity merged 9 commits intoAzure:masterfrom
samvaity:certs-track-2

Conversation

@samvaity
Copy link
Member

Based on the design that we agreed upon and documented here - https://github.com/g2vinay/KVSpec/blob/master/Certsv4.md

@adxsdk6
Copy link

adxsdk6 commented May 31, 2019

Can one of the admins verify this patch?

@samvaity samvaity requested a review from chlowell May 31, 2019 21:45
@samvaity samvaity self-assigned this May 31, 2019
@samvaity samvaity added Client This issue points to a problem in the data-plane of the library. KeyVault labels May 31, 2019
@samvaity samvaity requested a review from johanste May 31, 2019 21:47
# type: (str, CertificatePolicy) -> CertificatePolicy
pass

def update_certificate(self, name, version, not_before=None, expires=None, enabled=None, tags=None, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is not_before a boolean? Or is the #type annotation out-of-date?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated.

@samvaity samvaity marked this pull request as ready for review June 5, 2019 22:24
@samvaity samvaity requested a review from schaabs as a code owner June 5, 2019 22:24
issuer_id,
provider,
account_id,
password,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you certain none of these positional arguments could be optional? ("No, we can figure that out later" is a legitimate answer 😄)

Copy link
Member Author

@samvaity samvaity Jun 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not certain -true.
Will update this as I work on the implementation if the client.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have to look at the documentation. In many cases, the openapi description of the API will indicate if the value is required or not. In some cases, the value is required under some circumstances (e.g. only when you create a resource), in which case it may not be explicitly specified as required in the openapi specification.

In this case, it doesn't look like many of the attributes are actually required.

@samvaity samvaity requested a review from chlowell June 7, 2019 18:51
@samvaity samvaity merged commit 47fc3c4 into Azure:master Jun 10, 2019
rajivnandivada pushed a commit to rajivnandivada/azure-sdk-for-python that referenced this pull request Jul 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Client This issue points to a problem in the data-plane of the library. KeyVault

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants