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.

Sign up for GitHub

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

Jump to bottom

Prevent AttributeError during get_certificate_version #12747

Merged
merged 7 commits into from
Jul 29, 2020
Merged

Prevent AttributeError during get_certificate_version #12747

merged 7 commits into from
Jul 29, 2020

Conversation

chlowell
Copy link
Member

When a client GETs a specific version of a certificate, Key Vault doesn't return that certificate's management policy. So get_certificate_version calls KeyVaultCertificate._from_certificate_bundle with a bundle whose policy attribute is None, provoking an AttributeError because that code path assumes policy is a model instance. This PR prevents the error and adds test coverage around get_certificate_version and list_properties_of_certificate_versions. I also noticed that some lists in test_example_certificate_list_operations are empty, meaning these tests aren't as useful as they appear, and tweaked them to ensure this is no longer the case.

@chlowell chlowell added KeyVault Client This issue points to a problem in the data-plane of the library. blocking-release Blocks release labels Jul 27, 2020
@chlowell chlowell requested a review from iscai-msft July 27, 2020 21:17
@chlowell chlowell requested a review from schaabs as a code owner July 27, 2020 21:17
iscai-msft
iscai-msft previously approved these changes Jul 28, 2020
View reviewed changes
sdk/keyvault/azure-keyvault-certificates/tests/test_certificates_client.py Outdated
@ResourceGroupPreparer(random_name_enabled=True)
@KeyVaultPreparer()
@KeyVaultClientPreparer()
def test_versions(self, client, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: i wonder if it might be better to call it test_certificate_versions since I thought this had to do with API versions first

@chlowell chlowell requested a review from iscai-msft July 29, 2020 16:24
@chlowell chlowell merged commit 9b8d5c9 into Azure:master Jul 29, 2020
@chlowell chlowell deleted the list-cert-versions branch July 29, 2020 17:40
iscai-msft added a commit to iscai-msft/azure-sdk-for-python that referenced this pull request Jul 29, 2020
@iscai-msft
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-python …
2549e53 
…into regenerate_certs

* 'master' of https://github.com/Azure/azure-sdk-for-python: (45 commits)
  Prevent AttributeError during get_certificate_version (Azure#12747)
  re-apply updates after resetting master (Azure#12771)
  Sync eng/common directory with azure-sdk-tools repository (Azure#12745)
  Enable bandit for servicebus (Azure#12763)
  Skip pypy3 for service bus CI pipeline (Azure#12732)
  Update CODEOWNERS (Azure#12765)
  updated tox file (Azure#11087)
  Updating to match Microsoft recommended template (Azure#11045)
  [text analytics] update version in master (Azure#12749)
  Explicitly stringify message prints in samples per manual doc pass recommendation. (Azure#12709)
  Run only analyse dependency step for aggregate report (Azure#12728)
  VSCodeCredential supports Python 2.7 on Windows (Azure#12731)
  Enable bandit (Azure#12722)
  Move InteractiveCredential to a new module (Azure#12706)
  Add foundations for eng/common restructure (Azure#12725)
  Sync eng/common directory with azure-sdk-tools repository (Azure#12730)
  fix flaky test (Azure#12721)
  python link verification (Azure#12543)
  [ServiceBus] Remove legacy control client (Azure#12644)
  mirror update from MicrosoftDocs/azure-docs-sdk-python#709 (Azure#12691)
  ...
iscai-msft added a commit to iscai-msft/azure-sdk-for-python that referenced this pull request Jul 29, 2020
@iscai-msft
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-python …
40ae41c 
…into add_back_pii

* 'master' of https://github.com/Azure/azure-sdk-for-python: (40 commits)
  [text analytics] generate multiapi client, made v3.1-preview.1 default version (Azure#12720)
  Prevent AttributeError during get_certificate_version (Azure#12747)
  re-apply updates after resetting master (Azure#12771)
  Sync eng/common directory with azure-sdk-tools repository (Azure#12745)
  Enable bandit for servicebus (Azure#12763)
  Skip pypy3 for service bus CI pipeline (Azure#12732)
  Update CODEOWNERS (Azure#12765)
  updated tox file (Azure#11087)
  Updating to match Microsoft recommended template (Azure#11045)
  [text analytics] update version in master (Azure#12749)
  Explicitly stringify message prints in samples per manual doc pass recommendation. (Azure#12709)
  Run only analyse dependency step for aggregate report (Azure#12728)
  VSCodeCredential supports Python 2.7 on Windows (Azure#12731)
  Enable bandit (Azure#12722)
  Move InteractiveCredential to a new module (Azure#12706)
  Add foundations for eng/common restructure (Azure#12725)
  Sync eng/common directory with azure-sdk-tools repository (Azure#12730)
  fix flaky test (Azure#12721)
  python link verification (Azure#12543)
  [ServiceBus] Remove legacy control client (Azure#12644)
  ...
iscai-msft added a commit to iscai-msft/azure-sdk-for-python that referenced this pull request Jul 30, 2020
@iscai-msft
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-python …
7dc065c 
…into opinion_mining

* 'master' of https://github.com/Azure/azure-sdk-for-python: (69 commits)
  [formrecognizer] renames from consistency check (Azure#12752)
  [text analytics] add back PII endpoint (Azure#12673)
  [text analytics] generate multiapi client, made v3.1-preview.1 default version (Azure#12720)
  Prevent AttributeError during get_certificate_version (Azure#12747)
  re-apply updates after resetting master (Azure#12771)
  Sync eng/common directory with azure-sdk-tools repository (Azure#12745)
  Enable bandit for servicebus (Azure#12763)
  Skip pypy3 for service bus CI pipeline (Azure#12732)
  Update CODEOWNERS (Azure#12765)
  updated tox file (Azure#11087)
  Updating to match Microsoft recommended template (Azure#11045)
  [text analytics] update version in master (Azure#12749)
  Explicitly stringify message prints in samples per manual doc pass recommendation. (Azure#12709)
  Run only analyse dependency step for aggregate report (Azure#12728)
  VSCodeCredential supports Python 2.7 on Windows (Azure#12731)
  Enable bandit (Azure#12722)
  Move InteractiveCredential to a new module (Azure#12706)
  Add foundations for eng/common restructure (Azure#12725)
  Sync eng/common directory with azure-sdk-tools repository (Azure#12730)
  fix flaky test (Azure#12721)
  ...
iscai-msft added a commit to iscai-msft/azure-sdk-for-python that referenced this pull request Jul 30, 2020
@iscai-msft
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-python …
2bd6154 
…into regenerate_secrets

* 'master' of https://github.com/Azure/azure-sdk-for-python: (52 commits)
  [key vault] Regenerate keys (Azure#12101)
  [Tables] Azure Data Tables SDK, sync and async code (Azure#12766)
  fix doc, sample, docstring (Azure#12784)
  clean up links for PII samples (Azure#12826)
  [formrecognizer] renames from consistency check (Azure#12752)
  [text analytics] add back PII endpoint (Azure#12673)
  [text analytics] generate multiapi client, made v3.1-preview.1 default version (Azure#12720)
  Prevent AttributeError during get_certificate_version (Azure#12747)
  re-apply updates after resetting master (Azure#12771)
  Sync eng/common directory with azure-sdk-tools repository (Azure#12745)
  Enable bandit for servicebus (Azure#12763)
  Skip pypy3 for service bus CI pipeline (Azure#12732)
  Update CODEOWNERS (Azure#12765)
  updated tox file (Azure#11087)
  Updating to match Microsoft recommended template (Azure#11045)
  [text analytics] update version in master (Azure#12749)
  Explicitly stringify message prints in samples per manual doc pass recommendation. (Azure#12709)
  Run only analyse dependency step for aggregate report (Azure#12728)
  VSCodeCredential supports Python 2.7 on Windows (Azure#12731)
  Enable bandit (Azure#12722)
  ...
iscai-msft added a commit to iscai-msft/azure-sdk-for-python that referenced this pull request Jul 31, 2020
@iscai-msft
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-python …
6cd0f4f 
…into ta_opinion_mining_sample

* 'master' of https://github.com/Azure/azure-sdk-for-python: (47 commits)
  [text analytics] opinion mining support (Azure#12542)
  [key vault] Regenerate keys (Azure#12101)
  [Tables] Azure Data Tables SDK, sync and async code (Azure#12766)
  fix doc, sample, docstring (Azure#12784)
  clean up links for PII samples (Azure#12826)
  [formrecognizer] renames from consistency check (Azure#12752)
  [text analytics] add back PII endpoint (Azure#12673)
  [text analytics] generate multiapi client, made v3.1-preview.1 default version (Azure#12720)
  Prevent AttributeError during get_certificate_version (Azure#12747)
  re-apply updates after resetting master (Azure#12771)
  Sync eng/common directory with azure-sdk-tools repository (Azure#12745)
  Enable bandit for servicebus (Azure#12763)
  Skip pypy3 for service bus CI pipeline (Azure#12732)
  Update CODEOWNERS (Azure#12765)
  updated tox file (Azure#11087)
  Updating to match Microsoft recommended template (Azure#11045)
  [text analytics] update version in master (Azure#12749)
  Explicitly stringify message prints in samples per manual doc pass recommendation. (Azure#12709)
  Run only analyse dependency step for aggregate report (Azure#12728)
  VSCodeCredential supports Python 2.7 on Windows (Azure#12731)
  ...
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-python that referenced this pull request Feb 3, 2021
CodeGen from PR 12747 in Azure/azure-rest-api-specs
1a31f8d 
Adklinge/add is legacy log analytics rule (Azure#12747)

* Update scheduledQueryRule_API.json

Fixed Error contract to fit the real structure

* Update createOrUpdateScheduledQueryRules.json

Removed readonly properties from example parameters

* Update scheduledQueryRule_API.json

Added property "displayName" to the structure

* Fix to item ; https://portal.azure-devex-tools.com/amekpis/correctness/detail?errorId=A596EA9C-C8E9-4A72-90FE-4689DFCBA3F6

Fix to item ;
https://portal.azure-devex-tools.com/amekpis/correctness/detail?errorId=A596EA9C-C8E9-4A72-90FE-4689DFCBA3F6

* added display name to the new API version

* Fixed descriptions

* Added OverrideQueryTimeRange to Swagger on new API version

* returned enabled property back to bool

* Update scheduledQueryRule_API.json

Added missing enum values to ConditionalOperator property

* Update scheduledQueryRule_API.json

Aligned RP name to pascal format

* revert lindent correvction to avoid breaking chnage

* run prettier and fixed SubscriptionIdParameter

* revert subscripton id to the former description

* aligned  diagnosticSettings with master

* removed locaiton from example

* returned location to example

* supress false alaram OBJECT_ADDITIONAL_PROPERTIES error

* fix suppression

* fix merge conflict

* removed where clause

* added properties isLegacyLogAnalyticsRule, createdWithApiVersion to both api versions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iscai-msft iscai-msft iscai-msft approved these changes

@schaabs schaabs Awaiting requested review from schaabs

Assignees
No one assigned
Labels
blocking-release Blocks release Client This issue points to a problem in the data-plane of the library. KeyVault
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@chlowell @iscai-msft