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

[Service Bus] Normalize management naming with other languages #12829

Merged
merged 19 commits into from
Aug 6, 2020
Merged

[Service Bus] Normalize management naming with other languages #12829

merged 19 commits into from
Aug 6, 2020

Conversation

YijunXieMS
Copy link
Contributor

@YijunXieMS YijunXieMS commented Jul 30, 2020 •
edited
Loading

  1. QueueDescription to QueueProperties and QueueRuntimeInfo to QueueRuntimeProperties (ServiceBus - Track2 - Normalize management argument naming with other languages #12576 ). Same applies to Topic, Subscription and Rule
  2. Add scheduled_message_count to TopicRuntimeProperties and remove it from SubscriptionRuntimeProperties ([Service Bus] ATOM API: Add ScheduledMessageCount to TopicRuntimeProperties #12707 )
  3. Flatten MessageCountDetails and rename MessageCount -> TotalMessageCount
  4. Removed updatable properties from **kwargs of update_queue method. Users should update the input queue object. Same applies to Topic, Subscription and Rule
  5. create_queue now uses **kwargs instead of QueueProperties as input. Same for Topic, Subscription and Rule
  6. Other small adjustment

@YijunXieMS YijunXieMS added Service Bus Client This issue points to a problem in the data-plane of the library. labels Jul 30, 2020
This was linked to issues Jul 30, 2020
[Service Bus] ATOM API: Add ScheduledMessageCount to TopicRuntimeProperties #12707
Closed
ServiceBus - Track2 - Normalize management argument naming with other languages #12576
Closed
@YijunXieMS
Copy link
Contributor Author

YijunXieMS commented Jul 30, 2020 •
edited
Loading

/azp run python - servicebus - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@YijunXieMS YijunXieMS marked this pull request as ready for review August 3, 2020 21:23
@YijunXieMS YijunXieMS linked an issue Aug 3, 2020 that may be closed by this pull request
ATOM API update, should use distinct class from create for non-writeable fields #12414
Closed
@YijunXieMS YijunXieMS changed the title [Service Bus] Rename QueueDescription to QueueProperties and QueueRuntimeInfo to QueueRuntimeProperties [Service Bus] Normalize management naming with other languages Aug 3, 2020
@YijunXieMS YijunXieMS mentioned this pull request Aug 3, 2020
Closed
@yunhaoling
Copy link
Contributor

changelog is missing -- and we could carry the changelog for rule support changes into this PR

yunhaoling
yunhaoling reviewed Aug 4, 2020
View reviewed changes
sdk/servicebus/azure-servicebus/azure/servicebus/management/_models.py Outdated
Comment on lines 528 to 657
:ivar created_at: The exact time the queue was created.
:ivar created_at: The exact time the subscription was created.
:type created_at: ~datetime.datetime
:ivar updated_at: The exact time a message was updated in the queue.
:type updated_at: ~datetime.datetime
:ivar accessed_at: Last time a message was sent, or the last time there was a receive request
Copy link
Contributor

Choose a reason for hiding this comment

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

have we ever discussed about whether we should "_utc" suffix on the mgmt time related properties?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created issue #12900 to follow up. This needs discussion with other languages and other Python SDKs

yunhaoling
yunhaoling reviewed Aug 4, 2020
View reviewed changes
Copy link
Contributor

@yunhaoling yunhaoling left a comment

Choose a reason for hiding this comment

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

great work! just some comments on trivial stuff, one comment may apply to both sync, async and different entity types.

yunhaoling
yunhaoling reviewed Aug 4, 2020
View reviewed changes
sdk/servicebus/azure-servicebus/azure/servicebus/aio/management/_management_client_async.py Outdated
the properties you want to update. Only a portion of properties can be updated.
Refer to https://docs.microsoft.com/en-us/rest/api/servicebus/update-queue.

:param queue: The queue that is returned from `get_queue` and has the updated properties.
Copy link
Contributor

@yunhaoling yunhaoling Aug 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

can the "QueueProperties" returned from create_queue/list_queues be used to update queue? This question may also apply to other entities.

Copy link
Member

Choose a reason for hiding this comment

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

I would hope/assume the answer is yes across the board? At the end of the day the goal being "only able to update if you have the artifact representing a queue-that-already-exists and is thusly fully populated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the docstring to add create_queue and list_queues. Also added same thing to other entities.

yunhaoling
yunhaoling reviewed Aug 4, 2020
View reviewed changes
Copy link
Contributor

@yunhaoling yunhaoling left a comment •
edited
Loading

Choose a reason for hiding this comment

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

looks good, as discussed OOB, I'm good with changelog coming within another PR

@YijunXieMS YijunXieMS mentioned this pull request Aug 5, 2020
Closed
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@KieranBrantnerMagee
Copy link
Member

Approved assuming we've had a full livetest run :)

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@YijunXieMS YijunXieMS merged commit b866e13 into Azure:master Aug 6, 2020
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-python that referenced this pull request Feb 9, 2021
CodeGen from PR 12829 in Azure/azure-rest-api-specs
19eba0b 
[Hub Generated] Review request for Microsoft.Cache to add version stable/2021-03-01 (Azure#12829)

* Adds base for updating Microsoft.Cache from version preview/2020-10-01-preview to version 2021-03-01

* Updates readme

* Updates API version in new specs and examples

* Add support for persistence and fix other typos

* Fix SDK Warning: R2063 OperationIdNounConflictingModelNames

* Fix spellcheck error

* Fix acronym capitalization in descriptions

* Replace local parameters with common-types parameters

* Use naming convention consistent with Azure documentation

* Update word choice, remove deprecated x-ms-code-generation-setting, and fix operationId

* Add x-ms-secret extension and make TLS version a string enum

* Suppress errors about secrets being sent in responses
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-python that referenced this pull request Feb 22, 2021
CodeGen from PR 12829 in Azure/azure-rest-api-specs
22d8710 
[Hub Generated] Review request for Microsoft.Cache to add version stable/2021-03-01 (Azure#12829)

* Adds base for updating Microsoft.Cache from version preview/2020-10-01-preview to version 2021-03-01

* Updates readme

* Updates API version in new specs and examples

* Add support for persistence and fix other typos

* Fix SDK Warning: R2063 OperationIdNounConflictingModelNames

* Fix spellcheck error

* Fix acronym capitalization in descriptions

* Replace local parameters with common-types parameters

* Use naming convention consistent with Azure documentation

* Update word choice, remove deprecated x-ms-code-generation-setting, and fix operationId

* Add x-ms-secret extension and make TLS version a string enum

* Suppress errors about secrets being sent in responses
00Kai0 added a commit that referenced this pull request Feb 22, 2021
@00Kai0
T2 redisenterprise 2021 02 22 (#16846)
09521df 
* CodeGen from PR 12829 in Azure/azure-rest-api-specs
[Hub Generated] Review request for Microsoft.Cache to add version stable/2021-03-01 (#12829)

* Adds base for updating Microsoft.Cache from version preview/2020-10-01-preview to version 2021-03-01

* Updates readme

* Updates API version in new specs and examples

* Add support for persistence and fix other typos

* Fix SDK Warning: R2063 OperationIdNounConflictingModelNames

* Fix spellcheck error

* Fix acronym capitalization in descriptions

* Replace local parameters with common-types parameters

* Use naming convention consistent with Azure documentation

* Update word choice, remove deprecated x-ms-code-generation-setting, and fix operationId

* Add x-ms-secret extension and make TLS version a string enum

* Suppress errors about secrets being sent in responses

* test,version,CHANGELOG

* remove other usage

* fix version

Co-authored-by: SDKAuto <sdkautomation@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yunhaoling yunhaoling yunhaoling left review comments

@KieranBrantnerMagee KieranBrantnerMagee KieranBrantnerMagee approved these changes

@annatisch annatisch Awaiting requested review from annatisch

@zikalino zikalino Awaiting requested review from zikalino

Assignees
No one assigned
Labels
Client This issue points to a problem in the data-plane of the library. Service Bus
Projects
None yet
Milestone
No milestone
3 participants
@YijunXieMS @yunhaoling @KieranBrantnerMagee