Skip to content

[media] Fix "sync_keys" and "regenerate_key"#654

Closed
lmazuel wants to merge 4 commits intoAzure:masterfrom
lmazuel:media_fix
Closed

[media] Fix "sync_keys" and "regenerate_key"#654
lmazuel wants to merge 4 commits intoAzure:masterfrom
lmazuel:media_fix

Conversation

@lmazuel
Copy link
Copy Markdown
Member

@lmazuel lmazuel commented Nov 1, 2016

@amarzavery @BrianBlum for review


This change is Reviewable

Copy link
Copy Markdown

@BrianBlum BrianBlum left a comment

Choose a reason for hiding this comment

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

Need to also trim the comma after "OK"

@lmazuel
Copy link
Copy Markdown
Member Author

lmazuel commented Nov 3, 2016

@johanste There is no breaking change here:

  • "regenerate_keys" does not return anything, Python would have returned an invalid object at best, CloudError at worst
  • "keyType" is required, trying to call without it raise a CloudError

@johanste
Copy link
Copy Markdown
Member

johanste commented Nov 4, 2016

@lmazuel, switching a property from not required to required may be a breaking changes in some languages. We are just spinning up the breaking change review/notification process, and as of now, we are applying the tag if we think that something could be breaking in an SDK. Meaning we do expect false positives at the moment.

However, we very much appreciate your confirmation that this will not be breaking for the Python SDK :)

@lmazuel
Copy link
Copy Markdown
Member Author

lmazuel commented Nov 4, 2016

Adding "id" as required for "sync"
https://msdn.microsoft.com/en-us/library/azure/mt750386.aspx

@lmazuel
Copy link
Copy Markdown
Member Author

lmazuel commented Nov 4, 2016

@amarzavery @johanste @BrianBlum FYI:

  • To be precise about impact on the Python SDK, adding "required" changes a CloudError (refused by the RestAPI after the calls) to a "TypeError: missing parameters" (with no RestAPI calls). I don't call this "breaking change", since this doesn't change working code. (I understand this statement is for Python only)
  • I tested all operations and made a Python PR: azure-mgmt-media azure-sdk-for-python#855

@lmazuel lmazuel mentioned this pull request Nov 7, 2016
@amarzavery
Copy link
Copy Markdown
Contributor

@BrianBlum - I think the changes made in this PR are also made in #698. Hence, I think it will be safe to close this PR and let the other one get merged with all the changes.

@lmazuel lmazuel closed this Nov 28, 2016
@lmazuel lmazuel deleted the media_fix branch July 19, 2017 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants