-
Notifications
You must be signed in to change notification settings - Fork 162
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
Allow deployment slot settings #862
base: master
Are you sure you want to change the base?
Changes from 2 commits
f1ae6f1
b78a545
f529e22
849db95
106596d
de2b2e8
edc6aae
48626d1
8b9f7d9
de2fb64
3a882b7
7c8fcfc
6c136c2
aa9c093
47f8b38
8518bb5
87a72a1
934eaaf
f9de8d4
5dab8e7
63673cd
db67331
04d33a8
41864e0
561d3b7
31ff0ca
c3515dc
e5e4e70
3d0f1eb
cf09dfd
774e2d4
2168606
04bc033
e5943be
672ca19
f773c44
d23098f
7b0610b
a619428
97fec9a
ec78c6a
868787b
07e5284
8403226
c676c74
1647db4
5a8c297
6e80b44
c554b65
696806e
9afadfb
bbb7d87
8c8b484
2de2c8b
dcd0910
004ef55
0ae6dea
a06cae7
062fb5c
b60cc2a
ccc6404
2099a5d
1aa19ef
3e2592b
5cb9a58
3bd6686
4754169
8d2053b
6fea7c3
de90ac8
c45f736
1363c91
540a71e
019a1ee
522b3cf
9dcf395
42c2da0
78233b3
a9f32a4
6578a0f
6407710
9fce32a
f9d8cd8
77c2b5b
68a0fc1
7e281ce
d8cf05e
9058d23
cef4e04
c9bc1c8
c680a99
19a03d0
bcdf853
7917662
65b614c
b012a44
fd24ceb
3ac3498
e8ae29e
5f3ada6
5e34581
bf094e5
5737fc1
6a4233d
1d4eeb6
a2eed20
a2bce2c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Formatting - I actually do prefer this style but the rest of Farmer uses the more common record creation style, could you follow that? Also |
||
SiteName = this.Name.ResourceName; | ||
SlotSettingNames = this.CommonWebConfig.SlotSettingNames; | ||
} | ||
] | ||
|
||
type FunctionsBuilder() = | ||
|
@@ -343,7 +349,8 @@ type FunctionsBuilder() = | |
Slots = Map.empty | ||
WorkerProcess = None | ||
ZipDeployPath = None | ||
HealthCheckPath = None } | ||
HealthCheckPath = None | ||
SlotSettingNames = Set.empty } | ||
StorageAccount = derived (fun config -> | ||
let storage = config.Name.ResourceName.Map (sprintf "%sstorage") |> sanitiseStorage |> ResourceName | ||
storageAccounts.resourceId storage) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -178,7 +178,8 @@ type CommonWebConfig = | |
Slots : Map<string,SlotConfig> | ||
WorkerProcess : Bitness option | ||
ZipDeployPath : (string*ZipDeploy.ZipDeploySlot) option | ||
HealthCheckPath: string option } | ||
HealthCheckPath: string option | ||
SlotSettingNames: string Set } | ||
|
||
type WebAppConfig = | ||
{ CommonWebConfig: CommonWebConfig | ||
|
@@ -540,6 +541,12 @@ type WebAppConfig = | |
DomainName = customDomain | ||
SslState = SslDisabled } | ||
| NoDomain -> () | ||
|
||
if this.CommonWebConfig.SlotSettingNames <> Set.empty then | ||
{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As above re: record style. |
||
SiteName = this.Name.ResourceName; | ||
SlotSettingNames = this.CommonWebConfig.SlotSettingNames; | ||
} | ||
|
||
yield! (PrivateEndpoint.create location this.ResourceId ["sites"] this.PrivateEndpoints) | ||
] | ||
|
@@ -562,7 +569,8 @@ type WebAppBuilder() = | |
Slots = Map.empty | ||
WorkerProcess = None | ||
ZipDeployPath = None | ||
HealthCheckPath = None } | ||
HealthCheckPath = None | ||
SlotSettingNames = Set.empty } | ||
Sku = Sku.F1 | ||
WorkerSize = Small | ||
WorkerCount = 1 | ||
|
@@ -888,3 +896,16 @@ module Extensions = | |
[<CustomOperation "health_check_path">] | ||
/// Specifies the path Azure load balancers will ping to check for unhealthy instances. | ||
member this.HealthCheckPath(state:'T, healthCheckPath:string) = this.Map state (fun x -> {x with HealthCheckPath = Some(healthCheckPath)}) | ||
|
||
/// Adds slot settings | ||
[<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) } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Newline for each updated field please. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And space before and after |
||
|> this.Wrap state | ||
[<CustomOperation "slot_settings">] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The convention is to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given that |
||
member this.AddSlotSettings(state:'T, settings: (string*string) list) = | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this is needed independently of adding slots, it at least should support the same capabilities as the settings in slots by using the |
||
let current = this.Get state | ||
settings | ||
|> List.fold (fun (state:CommonWebConfig) (key, value: string) -> { state with Settings = state.Settings.Add(key, LiteralSetting value); SlotSettingNames = state.SlotSettingNames.Add(key) }) current | ||
|> this.Wrap state |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" ]} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could any of these tests be enhanced to make it a practical example that would show usage along with the existing DSL to |
||
|
||
let scn = functionsApp |> getResources |> getResource<Web.SlotConfigName> |> List.head | ||
let ws = functionsApp |> getResources |> getResource<Web.Site> |> List.head | ||
|
||
let template = arm{ add_resource functionsApp} | ||
let jobj = template.Template |> Writer.toJson |> Newtonsoft.Json.Linq.JObject.Parse | ||
|
||
let appSettingNames = | ||
jobj.SelectTokens $"$..resources[?(@name=='{functionsApp.Name.ResourceName.Value}/slotconfignames')].properties.appSettingNames[*]" | ||
|> Seq.map (fun x -> x.ToString()) | ||
|
||
let dependencies = | ||
jobj.SelectTokens $"$..resources[?(@name=='{functionsApp.Name.ResourceName.Value}/slotconfignames')].dependsOn[*]" | ||
|> Seq.map (fun x -> x.ToString()) | ||
|
||
let expectedSettings = Map [ | ||
"sticky_config", LiteralSetting "sticky_config_value" | ||
"another_sticky_config", LiteralSetting "another_sticky_config_value" ] | ||
|
||
let settings = Expect.wantSome ws.AppSettings "AppSettings should be set" | ||
Expect.containsAll settings expectedSettings "App settings should contain the slot settings" | ||
Expect.containsAll scn.SlotSettingNames ["sticky_config"; "another_sticky_config"] "Slot config names should be set" | ||
Expect.equal scn.SiteName (ResourceName "test") "Parent name should be set" | ||
Expect.containsAll appSettingNames [ "sticky_config"; "another_sticky_config"] "Slot config names should be present in template" | ||
Expect.containsAll dependencies [ $"[resourceId('Microsoft.Web/sites', '{functionsApp.Name.ResourceName.Value}')]"] "Slot config names resource should depend on web site" | ||
|
||
} | ||
|
||
test "Supports slot setting" { | ||
let functionsApp = functions { name "test"; slot_setting "sticky_config" "sticky_config_value" } | ||
|
||
let scn = functionsApp |> getResources |> getResource<Web.SlotConfigName> |> List.head | ||
let ws = functionsApp |> getResources |> getResource<Web.Site> |> List.head | ||
|
||
let template = arm{ add_resource functionsApp} | ||
let jobj = template.Template |> Writer.toJson |> Newtonsoft.Json.Linq.JObject.Parse | ||
|
||
let appSettingNames = | ||
jobj.SelectTokens $"$..resources[?(@name=='{functionsApp.Name.ResourceName.Value}/slotconfignames')].properties.appSettingNames[*]" | ||
|> Seq.map (fun x -> x.ToString()) | ||
|
||
let dependencies = | ||
jobj.SelectTokens $"$..resources[?(@name=='{functionsApp.Name.ResourceName.Value}/slotconfignames')].dependsOn[*]" | ||
|> Seq.map (fun x -> x.ToString()) | ||
|
||
let expectedSettings = Map [ | ||
"sticky_config", LiteralSetting "sticky_config_value" ] | ||
|
||
let settings = Expect.wantSome ws.AppSettings "AppSettings should be set" | ||
Expect.containsAll settings expectedSettings "App settings should contain the slot setting" | ||
Expect.containsAll scn.SlotSettingNames ["sticky_config"] "Slot config name should be set" | ||
Expect.equal scn.SiteName (ResourceName "test") "Parent name should be set" | ||
Expect.containsAll appSettingNames [ "sticky_config" ] "Slot config name should be present in template" | ||
Expect.containsAll dependencies [ $"[resourceId('Microsoft.Web/sites', '{functionsApp.Name.ResourceName.Value}')]"] "Slot config names resource should depend on web site" | ||
|
||
} | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder whether it's possible to piggy back on the existing
setting
keywords? Also, there's also thesecret_setting
keyword for adding secure settings - this should probably be migrated as well if we keep with separate keywords.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this really differ from including config when adding a slot with
add_slot
and using the SlotBuilder withaddSlot
? I see this PR addsslotconfignames
when you add these - is the purpose of this just for sticky config?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 haveslot_setting
which adds the setting and creates child resourceslotconfignames
.The alternative solution was something similar to this:
Where
slot_setting_names
was only marking the settings as sticky by addingslotconfignames
, But we thought the current solution withslot_setting
will make things clearer.