Skip to content

Init json quickstart updates#50

Merged
shawnallen85 merged 19 commits into
apache:masterfrom
shawnallen85:init-json-quickstart-updates
Apr 19, 2021
Merged

Init json quickstart updates#50
shawnallen85 merged 19 commits into
apache:masterfrom
shawnallen85:init-json-quickstart-updates

Conversation

@shawnallen85
Copy link
Copy Markdown
Contributor

Updated Init to decompress incoming binary in memory
Updated Quick Start to popd appropriately
Updated Newtonsoft.Json references for the runtimes

shawnallen85 and others added 16 commits October 17, 2019 15:53
Updated README.md to include license badge. (apache#19)
Updated .NET Core 2.2 Dockerfile to reference latest MS images
Migrated Quick Starts to individual projects
Moved shared test code to shared folder for reducing duplications
Removed trailing whitespace
Updating to xenial; dotnet sdk 3.0 install
Didn't update it, still referenced 14.04 dpkg
Updated to match expected formatting of file ... note for self: `gradlew scalafmtAll`
Updated to reflect appropriate release and release status.
Newtonsoft.Json update
Init decompression now in-memory
@shawnallen85 shawnallen85 requested a review from rabbah April 14, 2021 15:51
@rabbah
Copy link
Copy Markdown
Member

rabbah commented Apr 15, 2021

Thanks for engaging @mattwelke 🎉

@shawnallen85 shawnallen85 merged commit 9b96e80 into apache:master Apr 19, 2021
# Quick .NET Core 2.2 Action

A .NET Core action is a .NET Core class library with a method called `Main` that has the exact signature as follows:
A .NET Core action is a .NET Core class library with a method called `Main` or `MainAsync` that has the exact signature as follows:
Copy link
Copy Markdown

@mattwelke mattwelke Apr 20, 2021

Choose a reason for hiding this comment

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

Isn't this just a .NET convention, that async methods should end in "Async"? (https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/concepts/async/task-asynchronous-programming-model)

I'd argue that that should be up to the OpenWhisk user. In the end, I imagine the usefulness of this convention is that people writing code and using this function will see right away that it returns Task or Task<T>. In this case, they're coding a function that the runtime will use, so they don't get any value out of following that convention.

Right now, the runtime detects whether it needs to await the method, and if so, it does. This means the quickstarts before this PR can be used whether the user function is async or sync, and they're simpler that way because the user doesn't need to worry about following different paths depending on if they're writing an async or sync Main method.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These are just provided as examples of how you can create it -- it isn't gospel -- the user still has the option of defining the functions full name.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Because the user has the option to define the function name any way they like, I think the quickstarts should be about getting started as quick as possible with as few decisions to make as possible. Someone new to OpenWhisk is likely to be more interested in getting up and running easily than tweaking a function. They wouldn't even have a function yet.

`{Assembly}::{Class Full Name}::{Method}`, e.q.:

+ Synchronous: `Apache.OpenWhisk.Example.Dotnet::Apache.OpenWhisk.Example.Dotnet.Hello::Main`
+ Asynchronous: `Apache.OpenWhisk.Example.Dotnet::Apache.OpenWhisk.Example.Dotnet.Hello::MainAsync`
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

An example of the user having to make a decision where they technically don't need to, adding friction.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We could simplify the docs - to show the sync case with Main first because i think it's simpler.
Then describe the async case. The function can be called Main in both since the name isn't material.
I think you're right in that as shown it could lead one to think the name/Async suffix is semantically meaningful when it's not.

Copy link
Copy Markdown

@mattwelke mattwelke Apr 20, 2021

Choose a reason for hiding this comment

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

Glad to hear you agree. After looking again at @shawnallen85 's changes though, I agree there is one spot it's important to show both. That's at the very top where it says "that has the exact signature as follows", because in the async case, the signature is materially different.

I think to keep it simple, we should just use Main for the function name everywhere in the quickstart, and everywhere after that first part about the signature, just show one example regardless of sync/async, because the user would deploy the sync/async versions the exact same way with the exact same wsk commands.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do you want to take a shot at editing the quick start along these lines?
We do need to show the async case for sure, I've worked with enterprise devs who have asked how to write async examples.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Sure, this PR is already merged in, so I'll do a new PR when I have time (most of my OpenWhisking is done on the weekends right now) with suggested doc changes and ask for a review. I think for this PR, we're all set.

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.

3 participants