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

New Code Samples for Netherite #72

Merged
merged 2 commits into from
Sep 20, 2021
Merged

New Code Samples for Netherite #72

merged 2 commits into from
Sep 20, 2021

Conversation

lechnerc77
Copy link
Contributor

This pull request comprises additional code samples (TypeScript and Python) using Netherite in order to make the first steps easy for non-.NET users

  • Restructuring of setup
    • Moving of generic installation scripts into a dedicated folder
    • Rewriting of scripts for usage with node and python
    • reorganizing scripts (run and deployment are now contained in the folder for the languages)
  • Adding implementations for TypeScript and Python
  • Synchronizing implementations to be able to execute the functions:
    • locally without any Azure resource
    • hybrid with the Function running locally and the storage and Event Hub in Azure
    • deployed to Azure

It also contains some changes to the existing setup namely:

  • the settings.ps1 now throws an exception in case that the name is not adopted, stopping the execution
  • The .NET Function now only has a POST endpoint and authorization level is function
  • The creation of the storage account contains some more parameters that might prevent the execution due to typical security policies in Azure tenants

There will be an additional pull-request to adopt the GitHub pages to the new structure

@sebastianburckhardt sebastianburckhardt self-assigned this Sep 16, 2021
Copy link
Member

@sebastianburckhardt sebastianburckhardt left a comment

Choose a reason for hiding this comment

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

This looks great! Thanks a lot for creating these samples, I think they will be very helpful.

I had some minor comments and questions, see review. Also, I would suggest that you include the README.md in the typescript sample, since I found it very helpful.

@@ -25,7 +25,7 @@ public static class HelloCities
{
[FunctionName(nameof(HelloCities))]
public async static Task<IActionResult> Run(
[HttpTrigger(AuthorizationLevel.Anonymous, "get", "post", Route = "hellocities")] HttpRequest req,
[HttpTrigger(AuthorizationLevel.Function, "post", Route = "hellocities")] HttpRequest req,

Choose a reason for hiding this comment

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

Using non-anonymous access makes it slightly more difficult to run this sample. I suppose there is a risk is that users will forget to secure this later (after copying the sample).

Still, simplicity of the sample is important so I would probably leave this anonymous. I had a quick look at DF samples and they use anonymous also (e.g. https://github.com/Azure/azure-functions-durable-extension/blob/dev/samples/javascript/HttpStart/function.json).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had the risk in mind, but you are right, the samples in the documentation are anonymous. I changed that for all implementations. I will make a remark in the documentation

"scriptFile": "__init__.py",
"bindings": [
{
"authLevel": "function",

Choose a reason for hiding this comment

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

again, maybe use "anonymous"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

$Plan = "EP1",
$MinNodes = "1",
$MaxNodes = "20",
$Runtime = "python",

Choose a reason for hiding this comment

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

Is this script specific to python now? Or are you setting this parameter when calling this script?

Choose a reason for hiding this comment

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

Perhaps we could have runtime specific scripts that call this one, e.g. create-dotnet-function-app.ps1, create-python-function-app.ps1, create-typescript-function-app.ps1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I created three scripts now referencing the generic one.

$storageName = $name
$planName = $name

if (($name -eq "globally-unique-lowercase-alphanumeric-name-with-no-dashes")) {

Choose a reason for hiding this comment

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

I think this test condition should be updated to ($name -eq "testnetheritescripts") ... unless I am missing something.

Copy link
Contributor Author

@lechnerc77 lechnerc77 Sep 17, 2021

Choose a reason for hiding this comment

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

The other way round - did not change that back to your setup after my last test

### Start Orchestrator Function via HTTP for Python and NodeJS
POST http://localhost:7071/api/orchestrators/DurableFunctionsNetheriteOrchestrator

### Start Orchestrator Function via HTPP for .NET Core

Choose a reason for hiding this comment

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

HTPP -> HTTP

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@lechnerc77
Copy link
Contributor Author

Concerning the README-file. I will rework the documentation on the samples in the gh-pages branch and will take that into account

@lechnerc77
Copy link
Contributor Author

The documentation is now also updated see pull request #73

Copy link
Member

@sebastianburckhardt sebastianburckhardt left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks again.

I haven't tried running the samples on Linux yet, but at least on Windows it all works.

@sebastianburckhardt
Copy link
Member

@lechnerc77, I am ready to merge this and #73, unless you have some additional changes you want to add first.

@lechnerc77
Copy link
Contributor Author

@sebastianburckhardt : Nothing to add from my side - let the merging begin ;-)

@sebastianburckhardt sebastianburckhardt merged commit 0390605 into microsoft:main Sep 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants