Azure / azure-webjobs-sdk-extensions

Azure WebJobs SDK Extensions
MIT License
344 stars 206 forks source link

[.NET Core] - Migrate NotificationHubs extension project #239

Open fabiocav opened 7 years ago

fabiocav commented 7 years ago

Issue to track migration of the WebJobs.Extension.NotificationHubs project to .NET Standard 2.0

mathewc commented 7 years ago

I started this migration here https://github.com/Azure/azure-webjobs-sdk-extensions/tree/migrate_notification_hubs but hit a wall. They don't appear to have a .NET Standard package. Using the existing package results in a bunch of runtime load failures on various system assemblies.

mathewc commented 7 years ago

NotificationHubs team has started working on .Net Core 2.0 SDK for Notification Hubs. ETA: 09/29.

BorisWilhelms commented 6 years ago

Is there any update on this?

paulbatum commented 6 years ago

The notification hubs SDK for .net core 2.0 is available, so this porting work is unblocked. I don't have an ETA yet (right now we're focused on making sure the experience with our "core" bindings in V2 is smooth).

pragnagopa commented 6 years ago

@paulbatum - Are we tracking this item for GA?

pragnagopa commented 6 years ago

As of now, only preview version of NH nuget package is availablue https://www.nuget.org/packages/Microsoft.Azure.NotificationHubs/2.0.0-preview1

paulbatum commented 6 years ago

We are not tracking this for V2 GA as the NH package is still in preview and my understanding is that this won't change in the near term.

pragnagopa commented 6 years ago

https://www.nuget.org/packages/Microsoft.Azure.NotificationHubs/2.0.0 is released.

paulbatum commented 6 years ago

They changed their minds! Moving back into the backlog.

jsmarcus commented 6 years ago

Looking forward to having this extension for V2. Anything the community can do to help?

paulbatum commented 6 years ago

@jsmarcus Sorry for the delay, the answer is yes! I've made a very quick attempt at a port here: https://github.com/paulbatum/azure-webjobs-sdk-extensions/tree/notificationhubs

If you grab this branch, you should be able to generate a nuget by running dotnet pack .\src\WebJobs.Extensions.NotificationHubs\.

I have done ZERO testing with this. I just tried to get the code moved over and compiling. Any chance you're up for trying it out?

jsmarcus commented 6 years ago

@paulbatum I've tried it and am running into an issue. Where should I log it?

paulbatum commented 6 years ago

Lets just discuss it here since you're just working with bits on my fork.

jsmarcus commented 6 years ago

Sorry for the delay. Got caught up with other projects. Here is the exception I'm getting when trying to run my function app from command line.

Message=Unable to get constructor of NotificationHubsExtensionConfigProvider using provided constructor selector when resolving singleton NotificationHubsExtensionConfigProvider: IExtensionConfigProvider {ServiceKey=DefaultKey(4), ReturnDefaultIfNotRegistered} #105 in wrapper IEnumerable<IExtensionConfigProvider> as parameter "registeredExtensions" #1 in singleton DefaultExtensionRegistryFactory: IExtensionRegistryFactory #52

Full details here: https://gist.github.com/paulbatum/f7170955a46971456da2e69926af9d9d

paulbatum commented 6 years ago

hope you dont mind - I edited your reply to move the details into a gist since they were so large :)

I'm taking a look now..

paulbatum commented 6 years ago

Yeah there was an issue with the constructor visibility. I pushed a fix.

jsmarcus commented 6 years ago

This extension is working for me on my dev site. It's running as a v2.0 function app with incoming events from Event Grid.

One problem I had (which not everyone will have and isn't specifically related to your changes) was that I didn't want to specify my Hub Name in the attribute or host.json file. It took some work to figure out the correct app setting path to use. I don't think it's been documented (I haven't found it yet).

I am happy to share my project if you would like to see anything about it.

paulbatum commented 6 years ago

Thats great. I'm glad its at least in a semi-usable state now. Can you share with me how you setup your connection string and hub name? When I ported this code over I was doing a bit of guesswork myself. For example if you can share the name of the app setting you used to specify the hub, that would be useful.

jsmarcus commented 6 years ago

So I found that there are several ways to configure the settings. I am using deploy from zip with a csharp library so I wanted all my settings to be in AppSettings. But here are the options as far as I'm aware.

Configure via attribute This requires the hub name to be specified for every function and cannot be changed at runtime. Also, I've listed the default connection string setting name here but it can be left off or a different name can be specified. If the attribute ConnectionStringSetting is not null or empty, the value is looked up in ConnectionStrings, AppSettings, and Environment variables, in that order.

[NotificationHub(ConnectionStringSetting = "NotificationHubs", HubName = "<HubName>")] NotificationHubClient notificationHubClient

Configure via host.json & local.settings.json This allows the hub name to be specified in one place but it also cannot be changed at runtime for my specific project (csharp library/run from zip). The connection string part is the same as for the above attribute but I'll show the json for it so see my notes from above.

host.json

{
  "version": "2.0",
  "extensions": {
    "notificationHubs": {
      "hubName": "<HubName>"
    }
  }
}

local.settings.json note that both connection strings are not necessary

{
  "ConnectionStrings": {
    "NotificationHubs": "<ConnectionString>"
  }
  "IsEncrypted": false,
  "Values": {
    "NotificationHubs": "<ConnectionString>"
  }
}

Configure via AppSettings IOptions configuration loading This is the method I chose to use as it allows me to change both settings in AppSettings and they are in the same place. I'll show the local.settings.json but it ends up being AppSettings when hosted on Azure. This was also the hardest to figure out as it's not documented (or is very tough to find).

local.settings.json

{
  "IsEncrypted": false,
  "Values": {
    "AzureFunctionsJobHost:extensions:NotificationHubs:ConnectionString": "<ConnectionString>",
    "AzureFunctionsJobHost:extensions:NotificationHubs:HubName": "<HubName>"
  }
}
jsmarcus commented 6 years ago

Anything else I can do?

paulbatum commented 6 years ago

The next step is more mundane. I ported over the code but none of the unit tests. The tests for the old version live here: https://github.com/Azure/azure-webjobs-sdk-extensions/tree/v2.x/test/WebJobs.Extensions.Tests/Extensions/NotificationHubs

Our new project structure involves a separate test project for each extension as you can see here: https://github.com/Azure/azure-webjobs-sdk-extensions/tree/dev/test

So the next step involves creating a test project for notification hubs, porting the relevant tests over, confirming they run successfully, and checking to see if there are any test gaps (there might be a couple around how connection strings are handled since this changed in webjobs v3).

Let me know if you make any progress on this! Otherwise I think we should be able to move this forward on our side sometime during December.

jsmarcus commented 6 years ago

I've started working on these but I have run into a problem with not having a valid test connection string and hub name. I'll create a PR to your fork and I'll keep adding to it as I go. Hopefully you'll have a few minutes here or there to make sure I'm on the right path.

ChrisAllisonMalta commented 5 years ago

@paulbatum do you know if there is an estimate for when this will be available?

paulbatum commented 5 years ago

@ChrisAllisonMalta sorry for the lack of updates, this was deprioritized towards the end of last year and hasn't been picked back up. So while there is still no NuGet available, you could try to unblock yourself by building the bits in my fork (I provided instructions above). If you give this a try, it would be useful to hear whether the extension fulfills your scenario - the more reports we get that this is in a usable state, the better we can justify finishing the porting work and start publishing the nuget package.

ChrisAllisonMalta commented 5 years ago

Hi Paul, thanks for the note. I’ll give it a go, not sure my development skills are up to it but stranger things have happened!

In the meantime as far as serverless is concerned is the recommendation to use a v1 function in a separate project?

paulbatum commented 5 years ago

At this point in time my recommendation would be to use V2 and to reference the notification hubs client SDK instead of using an output binding. Obviously this is not ideal - output bindings typically require much less code to use. But it gets you on our latest bits and makes for a much simpler upgrade path once this work is complete.