Skip to content

[keyvault] add parse id feature for certs #13321

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
Closed
iscai-msft wants to merge 11 commits into from
Closed

[keyvault] add parse id feature for certs #13321

iscai-msft wants to merge 11 commits into from

Conversation

@iscai-msft
Copy link
Contributor

@iscai-msft iscai-msft commented Aug 25, 2020 •
edited
Loading

fixes #13006

I currently have the id property as original_id because for python we're not supposed to have properties / parameters titled id. Down to change this though (I see there's back and forth on @sadasant 's PR to maybe name this source_id too.

@ghost ghost added the KeyVault label Aug 25, 2020
heaths
heaths previously approved these changes Aug 26, 2020
View reviewed changes
@sadasant
Copy link
Contributor

sadasant commented Aug 27, 2020

@iscai-msft I'll review this after my PR gets approved and merged. Sorry for the delay!

@sadasant
Copy link
Contributor

sadasant commented Aug 31, 2020

@iscai-msft On TypeScript we will be using: parseKeyVaultCertificateId() and KeyVaultCertificateId. What do you think? I believe we should align 🌞

@iscai-msft
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-python …
05be0b7 
…into expose_parse_vault_id

* 'master' of https://github.com/Azure/azure-sdk-for-python: (37 commits)
  [text analytics] add versionadded sphinx documentation (Azure#13450)
  [text analytics] add bing_id property to LinkedEntity class (Azure#13446)
  fix typing for paging methods (Azure#13410)
  [text analytics] add domain_filter param (Azure#13451)
  fix issue Azure#11658 for is_valid_resource_id (Azure#11709)
  added create_table_if_not_exists method to table service client (Azure#13385)
  [ServiceBus] Test and failure improvements (Azure#13345)
  Proper encoding and decoding of source URLs - Fixes special characters in source URL issue (Azure#13275)
  Switch retry (Azure#13264)
  [ServiceBus] ServiceBusClient close spawned children (Azure#13077)
  fixing version issue by not overwriting the version with the semantic… (Azure#13411)
  clean up reference and tests (Azure#13412)
  Consistent returns (Azure#13245)
  [text analytics] return None for offset and length for v3.0 (Azure#13382)
  edit all authentication files and add a test (Azure#13355)
  Add managed_identity_client_id argument to DefaultAzureCredential (Azure#13218)
  [text analytics] add string-index-type support (Azure#13378)
  [text analytics] fix error response if pii entities is called from v3.0 client (Azure#13383)
  Send spec (Azure#13143)
  Anomaly Detector 3.0.0b2 release (Azure#13351)
  ...
@iscai-msft
Copy link
Contributor Author

iscai-msft commented Sep 1, 2020

@sadasant I'm going to keep the class shared across the different libraries (I can't think of a reason for users to instantiate their own class, so shouldn't run into namespace collisions). I'll make the method specific to each library though, i.e. for certs I'll do parse_key_vault_certificate_id

@iscai-msft iscai-msft requested a review from mccoyp September 1, 2020 16:03
heaths
heaths previously approved these changes Sep 1, 2020
View reviewed changes
mccoyp
mccoyp previously approved these changes Sep 2, 2020
View reviewed changes
chlowell
chlowell reviewed Sep 2, 2020
View reviewed changes
sdk/keyvault/azure-keyvault-certificates/tests/test_parse_id.py
from _shared.test_case import KeyVaultTestCase


class TestParseId(KeyVaultTestCase):
Copy link
Member

@chlowell chlowell Sep 2, 2020

Choose a reason for hiding this comment

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

These tests can be module-level methods.

sdk/keyvault/azure-keyvault-certificates/tests/test_parse_id.py Outdated
def test_parse_certificate_id_with_version(self):
# [START parse_key_vault_certificate_id]
source_id = "https://keyvault-name.vault.azure.net/certificates/certificate-name/version"
parsed_certificate_id = parse_key_vault_certificate_id(source_id)
Copy link
Member

@chlowell chlowell Sep 2, 2020

Choose a reason for hiding this comment

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

Parsing a literal seems odd in the snippet showing people how to use the method. This is what I imagine people would usually want to do:

cert = client.get_certificate("...")
parsed_id = parse_key_vault_certificate_id(cert.id)

Copy link
Contributor

@sadasant sadasant Sep 2, 2020

Choose a reason for hiding this comment

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

I like that!

@iscai-msft iscai-msft dismissed stale reviews from mccoyp and heaths via 057ee2d September 3, 2020 18:46
sadasant
sadasant reviewed Sep 3, 2020
View reviewed changes
Copy link
Contributor

@sadasant sadasant left a comment

Choose a reason for hiding this comment

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

Looks good to me 🌞

@iscai-msft
Merge branch 'master' of https://github.com/Azure/azure-sdk-for-python …
3de59c3 
…into expose_parse_vault_id

* 'master' of https://github.com/Azure/azure-sdk-for-python: (39 commits)
  [keyvault] fix include_pending param and 2016-10-01 compatibility (Azure#13161)
  don't want to exclude mgmt. auto-increments are fine here (Azure#13549)
  add test for opinion in diff sentence (Azure#13524)
  Change prerelease versioning (Azure#13500)
  [text analytics] add --pre suffix to pip install (Azure#13523)
  update changelog (Azure#13369)
  update release date for Sep (Azure#13370)
  update release date for Sep (Azure#13371)
  Sync eng/common directory with azure-sdk-tools repository for Tools PR 946 (Azure#13533)
  404 python erroring sanitize_setup. should not be (Azure#13532)
  Live pipeline issues (Azure#13526)
  [ServiceBus] Clean up README prior to P6 with doc-owner recommendations. (Azure#13511)
  Make doc-owner recommended revisions to eventhub readme and samples. (Azure#13518)
  Fix storage file datalake readme and samples issues (Azure#12512)
  [EventGrid] Receive Functions (Azure#13428)
  Tableitem metadata (Azure#13445)
  modify bing id docstring to mention Bing Entity Search more (Azure#13509)
  [text analytics] link opinion mining in the readme (Azure#13493)
  If match headers (Azure#13315)
  fix authorization header on asyncio requests containing url-encoded chars (Azure#13346)
  ...
mccoyp
mccoyp approved these changes Sep 4, 2020
View reviewed changes
Copy link
Member

@mccoyp mccoyp left a comment

Choose a reason for hiding this comment

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

Looks good! I'll just bump Charles's comment about tests

@iscai-msft
Copy link
Contributor Author

iscai-msft commented Sep 4, 2020

yep @mccoyp ended up half of @chlowell 's comment about tests. I had to keep it in a class though, since I need the test class inheritance to be able to come up with a random name for the cert I'm going to create

@mccoyp mccoyp mentioned this pull request Oct 15, 2020
Merged
@chlowell
Copy link
Member

chlowell commented Oct 26, 2020

This was superseded by #14518 (thanks @iscai-msft!)

@chlowell chlowell closed this Oct 26, 2020
@iscai-msft iscai-msft deleted the expose_parse_vault_id branch September 10, 2024 15:47
Copilot AI mentioned this pull request Dec 11, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mccoyp mccoyp mccoyp approved these changes

@heaths heaths heaths left review comments

@schaabs schaabs Awaiting requested review from schaabs

@g2vinay g2vinay Awaiting requested review from g2vinay

@chlowell chlowell Awaiting requested review from chlowell

+1 more reviewer

@sadasant sadasant sadasant left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

KeyVault

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[key vault] expose parser of KV ID

5 participants

@iscai-msft @sadasant @chlowell @heaths @mccoyp