Added knative support #967
Conversation
src/Azure.Functions.Cli/Actions/DeployActions/Platforms/KnativePlatform.cs
Outdated
Show resolved
Hide resolved
|
|
||
| var host = GetFunctionHost(name, nameSpace); | ||
|
|
||
| ColoredConsole.WriteLine(""); |
There was a problem hiding this comment.
nit: you don't need the ""
| ColoredConsole.WriteLine(""); | |
| ColoredConsole.WriteLine(); |
| File.Delete("deployment.json"); | ||
|
|
||
| var externalIP = await GetIstioClusterIngressIP(); | ||
| if (externalIP == "") |
There was a problem hiding this comment.
| if (externalIP == "") | |
| if (string.IsNullOrEmpty(externalIP)) |
| @@ -110,7 +110,7 @@ public override async Task RunAsync() | |||
| await DockerHelpers.DockerBuild(image, Environment.CurrentDirectory); | |||
|
|
|||
| ColoredConsole.WriteLine("Pushing function image to registry..."); | |||
| await DockerHelpers.DockerPush(image); | |||
| // await DockerHelpers.DockerPush(image); | |||
There was a problem hiding this comment.
is this intentional? I don't see the image being pushed anywhere else
| ColoredConsole.WriteLine("Plese note: it may take a few minutes for the knative service to be reachable"); | ||
| } | ||
|
|
||
| private string GetFunctionHost(string functionName, string nameSpace) { |
There was a problem hiding this comment.
nit: put the { on a new line.
| knativeService.spec.runLatest.configuration.revisionTemplate.metadata.annotations.Add("autoscaling.knative.dev/minScale", min.ToString()); | ||
| } | ||
|
|
||
| if (max > 0) { |
There was a problem hiding this comment.
nit: put the { on a new line.
| knativeService.spec.runLatest.configuration.revisionTemplate.metadata.annotations = new Dictionary<string, string>(); | ||
|
|
||
| // opt out of knative scale-to-zero for non-http triggers | ||
| if (!isHTTP) { |
There was a problem hiding this comment.
nit: put the { on a new line.
| { | ||
| var str = File.ReadAllText(string.Format("{0}/function.json", functionName)); | ||
| var jObj = JsonConvert.DeserializeObject<FunctionJson>(str); | ||
| return jObj.bindings.Any(d=> d.type == "http"); |
There was a problem hiding this comment.
I think you want httpTrigger, right?
There was a problem hiding this comment.
I was looking at the out binding, you're correct the in binding is what should be looked at
| client = KubeApiClient.Create(options); | ||
| } | ||
|
|
||
| private async Task Deploy(string name, string image, string nameSpace, int min, int max) |
There was a problem hiding this comment.
I'm a bit unclear about the use of this name here vs in the KubernetesPlatform.cs. In KubernetesPlatform it seems that name is used to name the deployment, while here, that name is used in the IsHttpTrigger method to check if that one function is in fact http, as well as used as a name for the deployment.
Shouldn't the check for IsHttpTrigger really be IsAnyHttpTrigger and then there enumerate all */function.json
There was a problem hiding this comment.
The deploy command deploys a single function, thus a single function.json
| } | ||
|
|
||
| private string GetFunctionHost(string functionName, string nameSpace) { | ||
| return string.Format("{0}.{1}.example.com", functionName, nameSpace); |
There was a problem hiding this comment.
is the example.com part a placeholder or would people deploying to knative understand what to replace that with?
There was a problem hiding this comment.
that's a fixed value in knative and can be expected.
…ePlatform.cs Co-Authored-By: yaron2 <yaronsc@microsoft.com>
|
@ahmelsayed all yours |
|
/cc @anirudhgarg for FYI |
This PR adds knative as a target platform to the 'func deploy' command.
Non-HTTP trigger workloads are supported - the minScale annotation is added to non-HTTP trigger functions in order to avoid scale-to-zero.