[EventGrid] Mappings for new ACS Events #14410
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The Azure Communication Services team noticed that some of their event shapes were wrong and have [updated the swagger](Azure/azure-rest-api-specs#13485) to address this. This commit pulls these changes into our SDK. Fixes Azure#14345
ellismg
requested review from
bterlson,
chradek,
ramya-rao-a and
xirzec
as code owners
ghost
added
the
Member
Author
Actually, after thinking about this more, I expect we are going to go to 4.1.0 even if we don't do a break, as we've added new members to the KnownSystemEvent enums and mapped a new event. |
This is a new event ACS is sending.
ellismg
force-pushed
the
ellismg/fix-14345
branch
from
f822d67 to
d184be3
Compare
After discussion, we are comfortable removing these two event names from our mapping. The rationale here is that the service never sent events using these names (they made a typo when documenting the event names in Swagger) and so any code using them was going to be wrong. In this case, we like that if you're using TypeScript you'll see a compile time issue here because it will be pointing to place in your code where things were never going to behave as you might have expected. The `CHANGELOG.md` has been updated to provide a little more perscriptive guidence on what to do here, and we feel OK about not doing a major version bump.
Co-authored-by: JoshLove-msft <[email protected]>
xirzec
approved these changes
Mar 23, 2021
Member
xirzec
left a comment
xirzec
left a comment
There was a problem hiding this comment.
Approved with a minor engsys-related comment.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Two things going on here, one simple, the other a little more complex:
Add a mapping for the ACS
RecordingFileStatusUpdatedevent. This is a new event that was added after our code freeze for GA. In addition, the ACS team has added a new property to some existing events.The ACS team discovered that the documentation for two of their events listed the wrong event type, and so our mappings were incorrect. I have added the new names to our list and mapped them to their corresponding shapes. I did not remove the older names, but added a
@deprecatedcomment to them, pointing at the new names.@xirzec @bterlson @ramya-rao-a The current status here is somewhat unfortunate. Code which was using the older names was never going to be correct (i.e.
isSystemEvent("Microsoft.Communication.ChatParticipantRemovedFromThread", e)will never returntrue, because no events use that event type name. But you can legally write that code today, which is somewhere surprising, since we try to constrain the first parameter to a known system event type. I think we have two options:Add some code to
isSystemEventthat would handle this case (i.e. treatisSystemEvent("Microsoft.Communication.ChatParticipantRemovedFromThread", e)as if you had written `isSystemEvent("ChatThreadParticipantRemoved") and do the same for the Added case. This would allow the code to behave as a user might expect.Remove
Microsoft.Communication.ChatParticipantRemovedFromThreadandMicrosoft.Communication.ChatParticipantAddedToThreadfrom our System Event Mappings. This would be a compile time breaking change for folks using TypeScript, but the break would point them at code which was logically incorrect, because the test they are doing will never return true.My preference is (2), on the basis that we've only had this library released for a week or so and the fact that this never actually worked the way customers who wrote the code expected. But I wouldn't want to bump the major version, per semver. I am not sure if this very localized break for TS users is something where we believe a good note in the
CHANGELOG.mdabout what we're doing (coupled with a bump to4.1.0instead of just4.0.1) would be sufficient.