Allow deployment slot settings #862

nargiz · 2022-01-18T11:41:22Z

This PR closes #838

The changes in this PR are as follows:

Allows you to specify slot settings for web app/functions using slot_setting and slot_settings

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:

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

let myWebApp = webApp {
    name "test"
    add_slot deploymentSlot

    slot_settings ["my_slot_setting", "sticky value"]
}

isaacabraham

Thank you very much! I think the core of this looks good, but I'm not sure adding a new keyword is the right way to go (see review comments).

I know that there were some other PRs and issues about slots - @ninjarobot @TheRSP do they relate to this at all?

isaacabraham · 2022-01-23T00:22:38Z

src/Farmer/Builders/Builders.Functions.fs

@@ -323,6 +323,12 @@ type FunctionsConfig =
                { site with AppSettings = None; ConnectionStrings = None }
                for (_, slot) in this.CommonWebConfig.Slots |> Map.toSeq do
                    slot.ToSite site
+
+            if this.CommonWebConfig.SlotSettingNames <> Set.empty then
+                {


Formatting - I actually do prefer this style but the rest of Farmer uses the more common record creation style, could you follow that? Also ; unnecessary and can be removed.

isaacabraham · 2022-01-23T00:23:43Z

docs/content/api-overview/resources/functions.md

@@ -45,6 +45,8 @@ The Functions builder is used to create Azure Functions accounts. It abstracts t
 | publish_as | Specifies whether to publish function as code or as a docker container. |
 | add_slot | Adds a deployment slot to the app |
 | add_slots | Adds multiple deployment slots to the app |
+| slot_setting | Sets a deployment slot setting of the function in the form "key" "value". |


I wonder whether it's possible to piggy back on the existing setting keywords? Also, there's also the secret_setting keyword for adding secure settings - this should probably be migrated as well if we keep with separate keywords.

How does this really differ from including config when adding a slot with add_slot and using the SlotBuilder with addSlot? I see this PR adds slotconfignames when you add these - is the purpose of this just for sticky config?

To make settings sticky, their names should be specified in webapp(sites) child resource, slotconfignames. We usually add settings in webapp, so the idea was to have slot_setting which adds the setting and creates child resource slotconfignames.

The alternative solution was something similar to this:

let myWebApp = webApp { name "test" add_slot deploymentSlot settings ["my_setting", "test value" "my_slot_setting", "sticky value"] slot_setting_names ["my_slot_setting" ] }

Where slot_setting_names was only marking the settings as sticky by adding slotconfignames, But we thought the current solution with slot_setting will make things clearer.

isaacabraham · 2022-01-23T00:24:18Z

src/Farmer/Builders/Builders.WebApp.fs

@@ -540,6 +541,12 @@ type WebAppConfig =
                  DomainName = customDomain
                  SslState = SslDisabled }
            | NoDomain -> ()
+
+            if this.CommonWebConfig.SlotSettingNames <> Set.empty then
+                {


As above re: record style.

isaacabraham · 2022-01-23T00:24:48Z

src/Farmer/Builders/Builders.WebApp.fs

+        [<CustomOperation "slot_setting">]
+        member this.AddSlotSetting (state:'T, key, value) =
+            let current = this.Get state
+            { current with Settings = current.Settings.Add(key, LiteralSetting value); SlotSettingNames =current.SlotSettingNames.Add(key) }


Newline for each updated field please.

And space before and after =

TheRSP · 2022-01-24T08:53:58Z

@isaacabraham this is the only open issue with slots that I'm aware of.
How would you propose this is achieved without the additional keyword? I can imagine a solution using a single-case-DU to tag a value as SlotSticky but I'm not sure this would be clearer?

…-extension Only turn on "auto" extension for Windows

ninjarobot · 2022-01-25T03:22:06Z

src/Farmer/Builders/Builders.WebApp.fs

+            { current with Settings = current.Settings.Add(key, LiteralSetting value); SlotSettingNames =current.SlotSettingNames.Add(key) }
+            |> this.Wrap state
+        [<CustomOperation "slot_settings">]
+        member this.AddSlotSettings(state:'T, settings: (string*string) list) =


If this is needed independently of adding slots, it at least should support the same capabilities as the settings in slots by using the Setting type instead of just strings. Then it can be a literal or an ARM expression.

ninjarobot · 2022-01-25T03:22:50Z

src/Farmer/Builders/Builders.WebApp.fs

+            let current = this.Get state
+            { current with Settings = current.Settings.Add(key, LiteralSetting value); SlotSettingNames =current.SlotSettingNames.Add(key) }
+            |> this.Wrap state
+        [<CustomOperation "slot_settings">]


The convention is to use add_ when adding items to the config, so this should be add_slot_settings.

Given that setting[s] doesn't follow that convention, would it be best if operator aligned with setting[s] or with other operators?

ninjarobot · 2022-01-25T03:24:20Z

src/Tests/Functions.fs

@@ -364,4 +364,63 @@ let tests = testList "Functions tests" [
            |> Seq.map (fun x-> x.ToObject<{|name:string;value:string|}> ())
        Expect.contains appSettings {|name="APPINSIGHTS_INSTRUMENTATIONKEY"; value="[reference(resourceId('shared-group', 'Microsoft.Insights/components', 'theName'), '2014-04-01').InstrumentationKey]"|} "Invalid value for APPINSIGHTS_INSTRUMENTATIONKEY"
    }
+
+    test "Supports slot settings" {
+        let functionsApp = functions { name "test"; slot_settings [ "sticky_config", "sticky_config_value"; "another_sticky_config", "another_sticky_config_value" ]} 


Could any of these tests be enhanced to make it a practical example that would show usage along with the existing DSL to add_slots?

…nctions

Add docker port setting for app service

…prestrictions

…aces Support for App Insights workspaces

isaacabraham · 2022-03-15T15:16:34Z

@nargiz I'm really sorry this hasn't been looked at for such a long time. We need to get this reviewed and merged in before it drifts too far from the main code base. @TheRSP @ninjarobot what do you both think about this - are there outstanding review issues that need to be resolved?

ninjarobot · 2022-03-15T23:35:55Z

I have some comments in the PR just around aligning with the existing DSL and making this fit with the concepts that are already there. It could be my misunderstanding of the intention, but I want to prevent DSL sprawl.

nargiz · 2022-03-16T11:22:20Z

Sorry, I got a bit confused with the DSL requirements, how slot_settings should be named/represented and distracted with other things. Thanks for reminding me. I will check it by the end of the week.

…tance

- add further LinkTo*Vnet overloads - update tests

Add support for VNet integration

…z-firewall AzureFirewall supports AvailabilityZones

martinbryant · 2023-08-18T11:41:27Z

Hi @nargiz - can we look at getting the conflicts dealt with so it can be reviewed? Thanks

viktorvan and others added 3 commits December 5, 2021 21:22

Add support for ip-restrictions for webApp/Functions.

f1ae6f1

allow slot_settings

b78a545

update slotSettingNames to be set

f529e22

nargiz changed the title ~~Comp it main~~ Allow deployment slot settings Jan 18, 2022

TheRSP added 3 commits January 21, 2022 11:54

Add support for multiple custom domains

849db95

Use domainName as certificate name

106596d

update docs + release notes

de2b2e8

isaacabraham reviewed Jan 23, 2022

View reviewed changes

isaacabraham self-requested a review January 23, 2022 00:26

isaacabraham and others added 4 commits January 23, 2022 01:54

Only turn on "auto" extension for Windows

edc6aae

Release notes

48626d1

Merge remote-tracking branch 'origin/master' into fix-linux-extension

8b9f7d9

Expanded description of custom_domain operator

de2fb64

Merge pull request CompositionalIT#867 from CompositionalIT/fix-linux…

3a882b7

…-extension Only turn on "auto" extension for Windows

ninjarobot reviewed Jan 25, 2022

View reviewed changes

Justin Wilkinson and others added 12 commits January 28, 2022 08:53

Fix host name binding error when deploying multiple custom domains

7c8fcfc

Add support for custom docker port for web apps

6c136c2

Update web app documentation for docker_port

aa9c093

Fix typo in App Setting (WEBSITE_PORT -> WEBSITES_PORT)

47f8b38

Make connection_string and connections_strings available for azure fu…

8518bb5

…nctions

Add connection string docs for Azure Functions

87a72a1

ZoneRedundant for service plan

934eaaf

more test cases for service plan

f9de8d4

Tiny cleanup

5dab8e7

Merge pull request CompositionalIT#870 from stevebelskie/master

63673cd

Add docker port setting for app service

Fix release notes

db67331

Merge branch 'master' of github.com:CompositionalIT/farmer into add-i…

04d33a8

…prestrictions

TheRSP and others added 4 commits March 15, 2022 08:36

Updated release notes

f9d8cd8

Update release notes and simplify tests.

77c2b5b

Merge remote-tracking branch 'origin/master' into ai-workspaces

68a0fc1

Merge pull request CompositionalIT#895 from CompositionalIT/ai-worksp…

7e281ce

…aces Support for App Insights workspaces

nargiz marked this pull request as draft March 16, 2022 11:19

bigjonroberts and others added 21 commits March 16, 2022 17:01

Merge branch 'master' into vm-spot-instance

d8cf05e

only allow one spot_instance or prioirity setting in VM

9058d23

correct some text on tests

cef4e04

Add docs about what spot_instance actually does.

c9bc1c8

Update release notes changes to match tag on PR

c680a99

Merge pull request CompositionalIT#894 from bigjonroberts/vm-spot-ins…

19a03d0

…tance

1.6.30 release

bcdf853

update to resemble to vm builder instead of AKS builder

7917662

merge

65b614c

- update docs

b012a44

- add further LinkTo*Vnet overloads - update tests

Add validation for functions

fd24ceb

AzureFirewall supports AvailabilityZones

3ac3498

Release notes

e8ae29e

Fix test failure due to increased validation

5f3ada6

Adds JSON selection test for zonal AzFirewall

5e34581

Merge pull request CompositionalIT#888 from codatio/vnet-integration

bf094e5

Add support for VNet integration

fix resource id for resource group config

5737fc1

Merge branch 'master' into ersoler/multiaz-firewall

6a4233d

Merge pull request CompositionalIT#900 from ericsoler1/ersoler/multia…

1d4eeb6

…z-firewall AzureFirewall supports AvailabilityZones

merge with latest

a2eed20

improvements for slot settings

a2bce2c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow deployment slot settings #862

Allow deployment slot settings #862

nargiz commented Jan 18, 2022 •

edited

Loading

isaacabraham left a comment

isaacabraham Jan 23, 2022

isaacabraham Jan 23, 2022

ninjarobot Jan 25, 2022

nargiz Jan 26, 2022

isaacabraham Jan 23, 2022

isaacabraham Jan 23, 2022

isaacabraham Jan 23, 2022

TheRSP commented Jan 24, 2022

ninjarobot Jan 25, 2022

ninjarobot Jan 25, 2022

TheRSP Mar 16, 2022 •

edited

Loading

ninjarobot Jan 25, 2022

isaacabraham commented Mar 15, 2022

ninjarobot commented Mar 15, 2022

nargiz commented Mar 16, 2022

martinbryant commented Aug 18, 2023

Allow deployment slot settings #862

Are you sure you want to change the base?

Allow deployment slot settings #862

Conversation

nargiz commented Jan 18, 2022 • edited Loading

isaacabraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheRSP commented Jan 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheRSP Mar 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaacabraham commented Mar 15, 2022

ninjarobot commented Mar 15, 2022

nargiz commented Mar 16, 2022

martinbryant commented Aug 18, 2023

nargiz commented Jan 18, 2022 •

edited

Loading

TheRSP Mar 16, 2022 •

edited

Loading