Skip to content

Conversation

@tiffanyachen
Copy link
Contributor

@tiffanyachen tiffanyachen commented Jan 24, 2018

This checklist is used to make sure that common issues in a pull request are addressed. This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.

PR information

  • The title of the PR is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
  • Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
  • If applicable, the PR references the bug/issue that it fixes.
  • Swagger files are correctly named (e.g. the api-version in the path should match the api-version in the spec).

Quality of Swagger

@mcardosos
Copy link
Contributor

Hello @tiffanyachen !
PR looks good, but there are merge conflicts :(

"operationId": "GetDeletedKeys",
"description": "List deleted keys in the specified vault. Authorization: Requires the keys/list permission.",
"summary": "Lists the deleted keys in the specified vault.",
"description": "Retrieves a list of the keys in the Key Vault as JSON Web Key structures that contain the public part of a deleted key. This operation icludes deletion-specific information. The Get Deleted Keys operation is applicable for vaults enabled for soft-delete. It requires the keys/list permission to be enabled on this vault. While the operation can be invoked on any vault, it will return an error if invoked on a non soft-delete enabled vault.",
Copy link

Choose a reason for hiding this comment

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

It requires the keys/list permission to be enabled on this vault [](start = 279, length = 64)

The caller needs the permission, not the permission is enabled on the vault.

"operationId": "GetDeletedKey",
"description": "Retrieves the deleted key information plus its attributes. Authorization: Requires the keys/get permission.",
"summary": "Gets the public part of a deleted key.",
"description": "The Get Deleted Key operation is applicable for soft-delete enabled vaults. It requires the keys/list permission to be enabled on this vault. While the operation can be invoked on any vault, it will return an error if invoked on a non soft-delete enabled vault.",
Copy link

Choose a reason for hiding this comment

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

It requires the keys/list permission to be enabled on this vault [](start = 100, length = 64)

The caller needs the permission, not the permission is enabled on the vault.

"operationId": "PurgeDeletedKey",
"description": "Permanently deletes the specified key. aka purges the key. Authorization: Requires the keys/purge permission.",
"summary": "Permanently deletes the specified key.",
"description": "The Purge Deleted Key operation is applicable for soft-delete enabled vaults. It requires the keys/purge permission to be enabled on this vault. While the operation can be invoked on any vault, it will return an error if invoked on a non soft-delete enabled vault.",
Copy link

Choose a reason for hiding this comment

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

It requires the keys/purge permission to be enabled on this vault [](start = 102, length = 65)

The caller needs the permission, not the permission is enabled on the vault.

"operationId": "RecoverDeletedKey",
"description": "Recovers the deleted key back to its current version under /keys. Authorization: Requires the keys/recover permission.",
"summary": "Recovers the deleted key to its latest version.",
"description": "The Recover Deleted Key operation is applicable for deleted keys in soft-delete enabled vaults. It recovers the deleted key back to its latest version under /keys. It requires the keys/recover permissions to be enabled on this vault. An attempt to recover an non-deleted key will return an error. Consider this the inverse of the delete operation on soft-delete enabled vaults.",
Copy link

Choose a reason for hiding this comment

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

It requires the keys/recover permissions to be enabled on this vault [](start = 188, length = 68)

The caller needs the permission, not the permission is enabled on the vault.

"summary": "List the versions of the specified secret.",
"description": "The LIST VERSIONS operation can be applied to all versions having the same secret name in the same key vault. The full secret identifier and attributes are provided in the response. No values are returned for the secrets and only current versions of a secret are listed.",
"summary": "List all versions of the specified secret.",
"description": "The full secret identifier and attributes are provided in the response. No values are returned for the secrets.",
Copy link

Choose a reason for hiding this comment

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

. [](start = 134, length = 1)

Should this mention that the operation requires the secrets/list permission?

"operationId": "PurgeDeletedSecret",
"description": "Permanently deletes the specified secret. aka purges the secret. Authorization: requires the secrets/purge permission.",
"summary": "Permanently deletes the specified secret.",
"description": "The purge deleted secret operation removes the secret permanently, without the possibility of recovery. It requires the secrets/purge permission, which must be assigned explicitly.",
Copy link

Choose a reason for hiding this comment

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

The purge deleted secret operation removes the secret permanently, without the possibility of recovery. It requires the secrets/purge permission, which must be assigned explicitly. [](start = 24, length = 180)

should this mention that it can only be performed on a soft delete enabled vault?

"operationId": "RecoverDeletedSecret",
"description": "Recovers the deleted secret back to its current version under /secrets. Authorization: requires the secrets/recover permission.",
"summary": "Recovers the deleted secret to the latest version.",
"description": "Recovers the deleted secret in the specified vault. It requires the secrets/recover permission.",
Copy link

Choose a reason for hiding this comment

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

Recovers the deleted secret in the specified vault. It requires the secrets/recover permission. [](start = 24, length = 95)

should this mention that it can only be performed on a soft delete enabled vault?

"operationId": "UpdateCertificate",
"summary": "Updates the specified attributes associated with the given certificate.",
"description": "The UpdateCertificate operation applies the specified update on the given certificate; note the only elements being updated are the certificate's attributes.",
"description": "The UpdateCertificate operation applies the specified update on the given certificate; the only elements updated are the certificate's attributes.",
Copy link

Choose a reason for hiding this comment

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

The UpdateCertificate operation applies the specified update on the given certificate; the only elements updated are the certificate's attributes. [](start = 24, length = 146)

should this mention that it requires the certificates/update permission

"operationId": "GetCertificate",
"description": "Gets information about a specified certificate. Authorization: requires the certificates/get permission.",
"summary": "Gets information about a certificate.",
"description": "Gets information about a specific certificate. It requires the certificates and get permissions.",
Copy link

Choose a reason for hiding this comment

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

It requires the certificates and get permissions. [](start = 71, length = 49)

this worded differently from other descriptions referring to required permissions

"summary": "Lists the deleted certificates in the specified vault, currently available for recovery.",
"description": "The GetDeletedCertificates operation retrieves the certificates in the current vault which are in a deleted state and ready for recovery or purging.",
"summary": "Lists the deleted certificates in the specified vault currently available for recovery.",
"description": "The GetDeletedCertificates operation retrieves the certificates in the current vault which are in a deleted state and ready for recovery or purging. This operation includes deletion-specific information. It requires the certificates/update permission.",
Copy link

Choose a reason for hiding this comment

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

The GetDeletedCertificates operation retrieves the certificates in the current vault which are in a deleted state and ready for recovery or purging. This operation includes deletion-specific information. It requires the certificates/update permission. [](start = 24, length = 251)

should this mention that it only applies to vaults with soft delete enabled?

Copy link

@schaabs schaabs left a comment

Choose a reason for hiding this comment

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

🕐

@azuresdkciprbot
Copy link

Hi There,

I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result:

File: specification/keyvault/data-plane/readme.md
Before the PR: Warning(s): 0 Error(s): 30
After the PR: Warning(s): 0 Error(s): 30

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@mcardosos
Copy link
Contributor

@schaabs
Is this ready to merge?

@tiffanyachen
Copy link
Contributor Author

Still updating for review @mcardosos

@azuresdkciprbot
Copy link

Hi There,

I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result:

File: specification/keyvault/data-plane/readme.md
Before the PR: Warning(s): 0 Error(s): 30
After the PR: Warning(s): 0 Error(s): 30

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

Copy link

@schaabs schaabs left a comment

Choose a reason for hiding this comment

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

LGTM

@mcardosos mcardosos merged commit 6180784 into Azure:master Jan 26, 2018
@AutorestCI
Copy link

This commit was treated and no generation was made for Python

@AutorestCI
Copy link

Swagger to SDK encountered an unknown error: (Azure/azure-sdk-for-go)

Traceback (most recent call last):
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/github_tools.py", line 29, in exception_to_github
    yield context
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/restapi/github.py", line 180, in rest_handle_action
    return rest_pull_close(body, github_con, restapi_repo, sdk_pr_target_repo, sdkbase)
  File "/usr/local/lib/python3.6/dist-packages/swaggertosdk/restapi/github.py", line 306, in rest_pull_close
    rest_pr.create_issue_comment("Was unable to create SDK %s PR for this closed PR.", sdkid)
TypeError: create_issue_comment() takes 2 positional arguments but 3 were given

@AutorestCI
Copy link

Did a commit to SDK for Python:
Azure/azure-sdk-for-python@573813f

@AutorestCI
Copy link

No modification for AutorestCI/azure-sdk-for-ruby

1 similar comment
@AutorestCI
Copy link

No modification for AutorestCI/azure-sdk-for-ruby

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.

5 participants