Skip to content

[event-grid][corev2] Convert EventGrid to use new core packages#12774

Merged
xirzec merged 7 commits intoAzure:masterfrom
xirzec:eventGridCoreV2
Jan 27, 2021
Merged

[event-grid][corev2] Convert EventGrid to use new core packages#12774
xirzec merged 7 commits intoAzure:masterfrom
xirzec:eventGridCoreV2

Conversation

@xirzec
Copy link
Member

@xirzec xirzec commented Dec 3, 2020

Here is a quick and dirty port over to the new core packages.

The only strange issue I ran into was the OperationSpec specifies a path of /api/events, but sample.env lists the endpoints as fully qualified:

EVENT_GRID_EVENT_GRID_SCHEMA_ENDPOINT="https://<event grid topic name>.<event grid topic region>.eventgrid.azure.net/api/events"

This results in URLs getting the path double-appended, so you have to redact /api/events in order for the client to work. I had a similar issue with the default template being https://{topicHostname} instead of just {topicHostname}.

Digging into this, it seems that our old URLBuilder class in core-http did a bunch of magic to save you from double-setting URL components (it would effectively reparse whatever component you gave it and try to set all discovered pieces individually.)

I think it's OK for us to not be quite so robust, but I'm curious what @ellismg thinks our story should be here. Perhaps we should drop the path from the OperationSpecs and allow the endpoint to be fully qualified instead?

@xirzec xirzec added Client This issue points to a problem in the data-plane of the library. Event Grid labels Dec 3, 2020
@xirzec xirzec self-assigned this Dec 3, 2020
@xirzec xirzec requested a review from ramya-rao-a December 3, 2020 23:02
@ellismg
Copy link
Member

ellismg commented Dec 4, 2020

I think it's OK for us to not be quite so robust, but I'm curious what @ellismg thinks our story should be here. Perhaps we should drop the path from the OperationSpecs and allow the endpoint to be fully qualified instead?

I feel like this is the correct strategy. Both the portal and and az cli say the entire URL (including the /api/events) is the service endpoint, so I expect that folks will want to use that entire string to construct the client.

@ellismg
Copy link
Member

ellismg commented Dec 4, 2020

FWIW, @xirzec, after looking over the diff, I really love the new client API. The places where things have changed make the code feel much more JS/TS like than before.

@xirzec xirzec changed the title [event-grid][corev2][DRAFT] Convert EventGrid to use new Pipeline model [event-grid][corev2] Convert EventGrid to use new core packages Jan 26, 2021
@xirzec xirzec marked this pull request as ready for review January 26, 2021 18:28
@xirzec xirzec requested a review from chradek as a code owner January 26, 2021 18:28
@xirzec
Copy link
Member Author

xirzec commented Jan 26, 2021

Marking this PR as ready to review, as we'd like to get this out on the new core beta packages for February.

/cc @ramya-rao-a

@ellismg
Copy link
Member

ellismg commented Jan 26, 2021

I think this will need a rebase because of #13361. I think they should be disjoint enough that the merge should not be too bad.

Could you let me know what if anything major changed between early December and now or any thing to look at in detail when I make another pass?

Excited to get our Feb beta out on this new model!

@xirzec
Copy link
Member Author

xirzec commented Jan 26, 2021

@ellismg I think we covered everything offline (mostly just _response going away.) I fixed the merge conflicts, but please do a sanity pass over the final diff so that I didn't accidentally break something not caught by tests. 😄

Copy link
Member

@ellismg ellismg left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks for doing this, Jeff!

@xirzec xirzec merged commit c291ab5 into Azure:master Jan 27, 2021
@xirzec xirzec deleted the eventGridCoreV2 branch January 27, 2021 23:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Client This issue points to a problem in the data-plane of the library. Event Grid

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants