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

Added support for running C# code in a dotnet core environment #84

Merged
merged 2 commits into from Jan 26, 2017

Conversation

Projects
None yet
3 participants
@ktrance
Copy link
Contributor

commented Jan 21, 2017

@soamvasani, as we discussed about over slack here is the dotnet environment implementation.

Be aware that the examples uses a container located at dockerhub under fission/dotnet-env which namespace I of course do not own, but I figured you guys might want build and add it there next to the other environments.

Please check README.dm for further description.

@ktrance

This comment has been minimized.

Copy link
Contributor Author

commented Jan 21, 2017

Any clue as to why the check fails?

To me it looks gofmt (I guess this is a Go linter) fails because it also looks into glide's (package manager?) cache as well but Im not really that well travelled in Go.

@soamvasani

This comment has been minimized.

Copy link
Member

commented Jan 21, 2017

Thanks for the PR! Excited to have C#/.NET support. Taking a look now.

The travis CI failure is indeed unrelated to your change, I will file another issue to track that.

response.StatusCode = HttpStatusCode.InternalServerError;
return response;
}
var args = ((DynamicDictionary)Request.Query).ToDictionary();

This comment has been minimized.

Copy link
@soamvasani

soamvasani Jan 22, 2017

Member

I suggest creating a "fission context" class and passing in an instance of that class. One property of that class can be the request object, plus it can have a method to get the querystring as a dictionary. That way we can add more stuff to the context later, without breaking compatibility with any functions.

This comment has been minimized.

Copy link
@ktrance

ktrance Jan 22, 2017

Author Contributor

Agreed that a context class is a better solution but I really do not like bleeding the Nancy API into the core implementation, it could be a problem later down the road if either we want to change hosting technology or if the Nancy guys decides to change their API.

Even if it might seem like a lot of extra plumbing I think the best way to proceed would be to create our own wrapper extracting the items from the request as we go along.

Other than the API concern then I think the context class should in abstract away the transport protocol as well.
I mean, do you as a consumer/developer really care about if the arguments to you need are delivered as a querystring over http get, as form data over https, binary serialized over tcp or protocol buffers? Probably not, I at least would not care as long as I got my arguments.
Do you see a use case where we would need access to for example the session and request headers in raw form in the future?

one more edit: exposing Nancy objects would also require users to add using statements from the Nancy module which I would really like to avoid. I would even consider not placing the FissionContext object in a namespace as it would just be one more thing for developers to remember.

Thoughts?

One last thing, have you considered what to do with logging yet? I would think maybe have a logging object on the context, for now just let it be a cover to write to stdout.

This comment has been minimized.

Copy link
@ktrance

ktrance Jan 22, 2017

Author Contributor

I added the changes I wrote about above as I figured it would be easier to discuss if we had some code to look at.

This comment has been minimized.

Copy link
@soamvasani

soamvasani Jan 23, 2017

Member

Thanks for adding the context class.

With the other environments (nodejs, python) we did not abstract away HTTP. So functions can access the full request object in both the python and nodejs environments. This was a conscious decision not just to reduce the complexity of the environments, but also to make it easier for function programmers to gather whatever info they want from the request. The web frameworks being used in those cases have stable and well documented APIs.

But, both those are dynamic/duck-typed languages so some of the concerns you're raising about declarations haven't come up before. I think one way to go would be to have the query string args easily parsed out as a dictionary, and the rest of the request object available to the function through some method. That way the most common use cases simply use the context.args dictionary and don't need any using declarations for nancy. What do you think?

For logging, stdout is good. The plan is to aggregate all logs from stdout into one of log aggregation systems. (stdout is also what shows up in kubectl logs, so it's good for users too.)

This comment has been minimized.

Copy link
@soamvasani

soamvasani Jan 23, 2017

Member

I forgot to answer the question about use cases for http headers. I think they're needed often. Here's a random example, if you're implementing a github webhook on fission, you may need access to these headers.

This comment has been minimized.

Copy link
@ktrance

ktrance Jan 23, 2017

Author Contributor

Fair enough, I would still really like to wrap the Nancy objects in something completely under our control but looking at the Nancy code its a larger tasks. One thing that should be considered is that most frameworks targeting dotnet core are still rather new or at least recently ported.
The Nancy API seem fairly stable but there are guarantees, sorry if I seem a little sceptic but I have been bitten too many times by breaking API changes and the problem is that once that genie is out of the bottle it can be very hard to put back in :)

Are you absolutely sure you are ready to take a dependency on NancyFx, if so I\ll add it?

This comment has been minimized.

Copy link
@soamvasani

soamvasani Jan 23, 2017

Member

Ok, this is a good discussion, I think we can keep the context free of nancy APIs for the moment and revisit the decision later if there is a use case that this context doesn't work for. As you say it's much easier to expand the API than shrink it. So I'm good with the context/request stuff as-is.

@@ -0,0 +1,158 @@
# Fission: dotnet C# Environment

This comment has been minimized.

Copy link
@soamvasani

soamvasani Jan 22, 2017

Member

Really appreciate the detailed documentation!

This comment has been minimized.

Copy link
@ktrance

ktrance Jan 22, 2017

Author Contributor

👍

Added basic logging, just a facade for now
Added FissionContext class
Changed name on both class and function for input code to avoid namespace clashes when adding FissionContext
Updated README
Added section on how to develop/debug the code to README, might be obvious for some but not all
Changed code input from static function to method
public Logger Logger { get; private set; }
}

public class Logger

This comment has been minimized.

Copy link
@soamvasani

soamvasani Jan 23, 2017

Member

Is there any existing lightweight logging library that we can use instead?

This comment has been minimized.

Copy link
@ktrance

ktrance Jan 23, 2017

Author Contributor

I'll look into it and get back to you. I just needed to know if you already had something so I should go look for a .net port of that. Give me a few days and I'll figure it out.

This comment has been minimized.

Copy link
@soamvasani

soamvasani Jan 23, 2017

Member

Sounds good, thanks. (Sorry for bringing up this issue late.)

This comment has been minimized.

Copy link
@haf

haf Jan 24, 2017

I've built a lightweight single-file logging implementation for C# called the Logary Facade. https://github.com/logary/logary#using-logary-in-a-library (scroll down for the C# example) You can then position an arbitrary logging framework behind the facade, should you wish to (like Logary itself)

This comment has been minimized.

Copy link
@ktrance

ktrance Jan 25, 2017

Author Contributor

@haf : Thanks, I'll have a look hopefully tomorrow just a little bit booked right now.

This comment has been minimized.

Copy link
@ktrance

ktrance Jan 26, 2017

Author Contributor

@haf , the link to the .cs file is broken

This comment has been minimized.

Copy link
@haf

haf Jan 26, 2017

@ktrance Thanks, I've fixed that link now. Thanks for telling me.

@soamvasani soamvasani merged commit 7485dca into fission:master Jan 26, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.