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

Add option to set DAPR protocol for Container Apps #1100

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

fatim
Copy link

@fatim fatim commented Apr 1, 2024

This PR closes #1083

The changes in this PR are as follows:

  • Container Apps: Add support for setting Dapr protocol.

I have read the contributing guidelines and have completed the following:

  • Tested my code end-to-end against a live Azure subscription.
  • Updated the documentation in the docs folder for the affected changes.
  • Written unit tests against the modified code that I have made.
  • Updated the release notes with a new entry for this PR.
  • Checked the coding standards outlined in the contributions guide and ensured my code adheres to them.

If I haven't completed any of the tasks above, I include the reasons why here:

I don't have experience using container apps with Dapr in Azure.
This change produces correct ARM config, I've used this doc as a reference - https://learn.microsoft.com/en-us/azure/container-apps/enable-dapr?tabs=arm1
Could somebody test this in Azure.

Below is a minimal example configuration that includes the new features, which can be used to deploy to Azure:

let httpContainerApp =
    containerApp {
        ...
        // Dapr config
        dapr_app_id "http"
        dapr_app_port 5000us

        // new parameter added by this PR
        dapr_app_protocol "grpc"
        ...
    }

@jwthomson
Copy link
Member

Thanks for this contribution!

This looks great in principle. @isaacabraham is going to have a look at it, apologies for the delay.

Copy link
Member

@isaacabraham isaacabraham 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 - just a small change which will add some type safety and improve usability.

@@ -39,7 +39,8 @@ type ContainerAppConfig =
Identity: ManagedIdentity
Replicas: {| Min: int; Max: int |} option
DaprConfig: {| AppId: string option
Port: uint16 option |} option
Port: uint16 option
Protocol: string option |} option
Copy link
Member

Choose a reason for hiding this comment

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

I would make a type for the protocol e.g.

[<RequireQualifiedAccess>]
type DaprProtocol = Grpc | Http

Doing this will make it easier for individuals to know what the available valid options are, rather than a raw string.

@@ -313,6 +315,7 @@ type ContainerApp =
enabled = true
appId = settings.AppId
appPort = settings.Port |> Option.toNullable
appProtocol = settings.Protocol
Copy link
Member

Choose a reason for hiding this comment

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

With Protocol now a union rather than a string, this code will need a pattern match to emit the correct string e.g.

match settings.Protocol with Http -> "http" | Gprc -> "grpc"

etc.

@mattgallagher92
Copy link
Member

Hi @fatim, thanks for your contribution 🙂 Would you like to make the changes that Isaac suggested, or would you prefer that someone else picks up this PR?

@isaacabraham
Copy link
Member

Let's give this another week - if it's not worked on, we should close this PR but add an issue to revisit it at a later date.

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.

Add option to set DAPR protocol for Container Apps
4 participants