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.

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

Sync the _shared folder for Communication #21777

Merged
merged 5 commits into from
Jan 12, 2022
Merged

Sync the _shared folder for Communication #21777

merged 5 commits into from
Jan 12, 2022

Conversation

petrsvihlik
Copy link
Contributor

@petrsvihlik petrsvihlik commented Nov 16, 2021

I found out that the _shared folder (sdk\communication\azure-communication-<package>\azure\communication\<package>\_shared) is not in sync across the Communication packages (SMS, PhoneNumbers, Chat, Identity, and NetworkTraversal).

The differences made our _shared folders split into the following branches:

  • Branch A:
    • PhoneNumbers
    • SMS
    • Chat (1 difference compared to SMS and PhoneNumbers)
  • Branch B:
    • Identity
    • NetworkTraversal (1 little change related to linting compared to Identity)

@jbeauregardb attempted to unify the folders in the past (#17985) but Identity was not updated (and I think NetworkTraversal was introduced later and that's how Branch B was created).

The second important milestone was #18026 that removed replace(tzinfo=TZ_UTC) from creation of the current timestamp.

  • There was a comment saying that we "should have some unit tests including both naïve and TZ-aware inputs." which wasn't processed. I'll probably add some tests for that.
  • Also, it'd probably make sense to be able to handle both the naïve and TZ-aware inputs in the code to be compatible with the older versions of Python. E.g. similarly to what they do in Azure.Core. I'll take a closer look at it and suggest a change.

This PR is an attempt to cherry pick the right changes and unify the folders.
I'm including all past contributors and owners of concerned packages as reviewers.

@ghost ghost added the Communication label Nov 16, 2021
@azure-sdk
Copy link
Collaborator

API changes have been detected in azure-communication-chat. You can review API changes here

API changes

- # Package is parsed using api-stub-generator(version:0.2.4)
+ # Package is parsed using api-stub-generator(version:0.2.5)
-             metadata: Dict = None, 
-             sender_display_name: Union[str, NoneType] = None, 
-             content: str = None, 
+             content = ..., 
-             content = ..., 
-             metadata: Dict = None, 

@azure-sdk
Copy link
Collaborator

API changes have been detected in azure-communication-sms. You can review API changes here

API changes

- # Package is parsed using api-stub-generator(version:0.2.4)
+ # Package is parsed using api-stub-generator(version:0.2.5)
-         ) -> SmsClient
+         ) ->  admonitio
-         ) -> SmsClient
+         ) ->  admonitio

@azure-sdk
Copy link
Collaborator

API changes have been detected in azure-communication-phonenumbers. You can review API changes here

API changes

- # Package is parsed using api-stub-generator(version:0.2.4)
+ # Package is parsed using api-stub-generator(version:0.2.5)

@petrsvihlik
Copy link
Contributor Author

Since we don't expose the value of datetime.utcnow() anywhere outside the get_current_utc_as_int(), I think we can keep it without the .replace(tzinfo=TZ_UTC). I added a test for UTC without a timezone specified to make sure the _convert_datetime_to_utc_int works in all cases.
I ran the tests on both Python 2.x and 3.x and all seems to be passing.

@juancamilor
Copy link
Member

@LuChen-Microsoft FYI

@petrsvihlik petrsvihlik mentioned this pull request Nov 19, 2021
12 tasks
@petrsvihlik
Copy link
Contributor Author

petrsvihlik commented Nov 26, 2021

Added a new line to CODEOWNERS to enforce the reviews for the _shared folders bbaff76 and prevent similar situation from happening again.

@petrsvihlik
Copy link
Contributor Author

Also, it'd probably make sense to be able to handle both the naïve and TZ-aware inputs in the code to be compatible with the older versions of Python. E.g. similarly to what they do in Azure.Core. I'll take a closer look at it and suggest a change.

It seems that TZ_UTC covers this well.

I also did some extensive testing and discovered that deserialization of the expires_on doesn't work correctly - there was a 1hr difference between the string payload and the deserialized int. So in 5371ea7, I replaced this:

return AccessToken(token, _convert_datetime_to_utc_int(datetime.fromtimestamp(payload['exp']).replace(tzinfo=TZ_UTC)))

with:

return AccessToken(token, _convert_datetime_to_utc_int(datetime.fromtimestamp(payload['exp'], TZ_UTC)))

Now it should be all good to review.

@AriZavala2 @LuChen-Microsoft @RoyHerrod @jbeauregardb can you please verify the changes?

@petrsvihlik
Copy link
Contributor Author

Confirmed with @annatisch that this change is legit. Explanation:

token types follow a specific protocol, so the method signatures needed to be updated to be consistent.

image

This PR unifies the code to follow the change made in the Chat lib.

@annatisch
Copy link
Member

/azp run python - azure-communication-chat - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@annatisch
Copy link
Member

/azp run python - azure-communication-phonenumbers - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@annatisch
Copy link
Member

/azp run python - azure-communication-sms - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@annatisch
Copy link
Member

/azp run python - azure-communication-identity - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@annatisch
Copy link
Member

/azp run python - azure-communication-networktraversal - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@petrsvihlik petrsvihlik merged commit cdeb608 into Azure:main Jan 12, 2022
rakshith91 pushed a commit to rakshith91/azure-sdk-for-python that referenced this pull request Apr 10, 2022
* identity and network traversal unified with sms

* unified get_current_utc_as_int + added a test

* adjusted variable name to match the comment

* added owners to enforce reviews

* token deserialization fix + test
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.

7 participants