Take in Azure/Moodle changes to azure-quickstart-templates #4514

hosungsmsft · 2018-04-04T20:40:52Z

Best Practice Checklist

Check these items before submitting a PR... See the Contribution Guide for the full detail: https://github.com/Azure/azure-quickstart-templates/blob/master/1-CONTRIBUTION-GUIDE/README.md

uri's compatible with all clouds (Stack, China, Government)
Staged artifacts use _artifactsLocation & _artifactsLocationSasToken
Use resourceGroup().location for resource locations
Folder names for artifacts (nestedtemplates, scripts, DSC)
Use literal values for apiVersion (no variables)
Parameter files (GEN-UNIQUE for value generation and no "changemeplease" values
$schema and other uris use https
Use uniqueString() whenever possible to generate names for resources. While this is not required, it's one of the most common failure points in a deployment.
Update the metadata.json with the current date

For details: https://github.com/Azure/azure-quickstart-templates/blob/master/1-CONTRIBUTION-GUIDE/bp-checklist.md

- Please check this box once you've submitted the PR if you've read through the Contribution Guide and best practices checklist.

Changelog

Take in changes made in our development repo since last updates. Notable changes are as follows:
- Added more configuration switches, like redis on/off, various Moodle plugins, HTML local copy feature, ...
- Added Microsoft SQL server on Azure as a DB option
- Updated Azure MySQL/PostgreSQL APIs for GA
- Various other improvements

hosungsmsft · 2018-04-04T20:41:42Z

@rgardler -- FYI

SorraTheOrc · 2018-04-09T20:38:53Z

+1

bmoore-msft · 2018-04-16T23:23:25Z

moodle-scalable-cluster-ubuntu/azuredeploy.json

+            "type": "bool"
+        },
+        "siteURL": {
+            "defaultValue": "www.example.org",


is this valid for all deployments?

Yes. The install scripts take either an empty value or this default value to mean "configure it for me". In this situation the scripts generate a DNS name based on the LB name and create a self-signed certificate. This behavior is documented (not that I expect you to read the docs in a review, just letting you know it is documented).

sounds good

bmoore-msft · 2018-04-16T23:23:46Z

moodle-scalable-cluster-ubuntu/azuredeploy.json

+            "type": "string"
+        },
+        "sshUsername": {
+            "defaultValue": "azureadmin",


remove this - increases surface area

We took the approach of providing as many reasonable default values as possible to minimize the number of unfilled required params, allowing beginning customers to try out the templates without having to figure out all the parameters. Once they get confidence and ready to deploy for their production, they can customize all these parameters (just like the siteURL parameter as above). I think we'd like to keep it this way, as it has always been. @rgardler -- Would you agree?

I would agree.

Right approach, but not with something that compromises security... having a default username makes it easier to attack the app

this still needs to be removed

bmoore-msft · 2018-04-16T23:24:04Z

moodle-scalable-cluster-ubuntu/azuredeploy.json

-        "firewallRuleName": {
-            "defaultValue": "open-to-the-world",
+        "dbLogin": {
+            "defaultValue": "dbadmin",


remove this increases surface area

Same response as above...

also needs to be removed

bmoore-msft · 2018-04-16T23:25:19Z

moodle-scalable-cluster-ubuntu/azuredeploy.json

@@ -587,6 +678,7 @@
            "dbServerType": "[parameters('dbServerType')]",
            "dbUsername": "[concat(parameters('dbLogin'), '@', parameters('dbServerType'), '-', variables('resourceprefix'))]",
            "elasticVmSku": "[parameters('elasticVmSku')]",
+            "dbDNS": "[if(equals(parameters('dbServerType'),'mssql'),concat(parameters('dbServerType'), '-',variables('resourcePrefix'), '.database.windows.net'), concat(parameters('dbServerType'), '-', variables('resourcePrefix'), '.', parameters('dbServerType'), '.database.azure.com'))]",


use reference() to get the FQDNs

According to the documentation, we can't use reference() in the variables section (mentioned in Remarks). Is this changed?

you can't you need to move this into the properties section that needs it.

Fixed in the newly added commit

bmoore-msft · 2018-04-16T23:25:55Z

moodle-scalable-cluster-ubuntu/azuredeploy.json

@@ -394,7 +476,7 @@
                    }
                },
                "templateLink": {
-                    "uri": "[concat(variables('moodleCommon').baseTemplateUrl,'redis.json',parameters('_artifactsLocationSasToken'))]"
+                    "uri": "[concat(variables('moodleCommon').baseTemplateUrl,'redis', if(parameters('redisDeploySwitch'), '', '-none'), '.json',parameters('_artifactsLocationSasToken'))]"


any reason we can't use the condition property for this?

That's exactly what I tried at the beginning, and it kept failing, leading me to conclude that the ARM's if(,,) function is eager and causes this issue. Let me explain further in detail.

Basically, I wanted something like the following for the redisKey value in controller.json:

if([condition],listKeys([redisResourceId]).primaryKey),'None')

The [condition] would be true iff the redisDeploySwitch parameter is true. However, even when the switch is false, ARM keeps evaluating the listKeys([redisResourceId]).primaryKey) and complaining that there's no such resource (because no redis will be deployed when the switch is false). This was unexpected to me (as I was expecting a programming-language-like behavior with if(,,)), but then I realized that the parameter evaluations in if(,,) might be eager, call-by-value, and all 3 parameters are evaluated even if the condition is false, which is clearly different from the usual programming-language-like behavior of if ... else ... construct.

I've been wondering if this understanding of mine is correct, and who to ask about this. Could you confirm if this is the currently expected behavior? If not, could you let me know who to ask about this?

Bottom line is that I did/do want to get rid of this hack, but the ARM's if(,,) semantics I experienced doesn't allow that, so I couldn't avoid this hack. Please advise.

try again - we released a bug fix in this areas not long ago - if it still fails, send me the code that's failing

Thanks. It's working now as desired and fixed in the newly added commit.

bmoore-msft · 2018-04-16T23:27:45Z

moodle-scalable-cluster-ubuntu/azuredeploy.parameters.json

-                "storageAccountType":          { "value": "Standard_LRS"},
-                "controllerVmSku":             { "value": "Standard_DS1_v2" },
-                "dbServerType":                { "value": "mysql" },
+                "redisDeploySwitch":           { "value": false },


remove the non GEN values here and just use defaultValues in the template

Actually, the default value change to redisDeploySwitch (or any other params) requires some internal discussion (@rgardler -- please chime in). We chose the default values in azuredeploy.json with some direction in mind, but here in the parameters file, we need to change it to minimize the test deployment time, so that's why it's changed here.

@rgardler , I'm in favor of changing this default to false in azuredeploy.json, but I wouldn't do that without your agreement first.

I'm happy to follow your guidance on this one. My reading of the load testing results is that Reddis is not needed in anything but the most active scenarios. In which case I would support false by default.

Thanks @rgardler . Updated the default, parameters.json, and pushed the commit.

bmoore-msft · 2018-04-16T23:28:01Z

moodle-scalable-cluster-ubuntu/docs/Cleanup.md

@@ -0,0 +1,22 @@
+# Cleanup All Resource


bmoore-msft · 2018-05-22T18:43:56Z

@hosungsmsft - can you address the merge conflicts?

hosungsmsft · 2018-05-22T19:14:10Z

@bmoore-msft -- Your commit 0552ef8 introduced a whole lot of merge conflicts (mostly for tab width, but also for introducing the location parameter to all template .json files), which will take some time for me to resolve (I'm engaged right now with some urgent customer support case). The change will also have to come from our Azure/Moodle repo, so that's how I'll resolve these conflicts later.

hosungsmsft · 2018-05-23T21:25:29Z

@bmoore-msft -- Merge conflicts are now resolved. I modified your location parameter change so that it aligns with linked templates (all of the linked/nested resources should be all in the same resource group, not individually specify-able).

hosungsmsft · 2018-06-22T00:52:32Z

@bmoore-msft -- Brian, can we get this PR finally merged? I know there were repeated merge conflicts due to this PR being opened too long (your ongoing changes on this long running PR kept causing merge conflicts, even after I resolved them manually by merging using 'theirs' strategy). I just reset my master to the current upstream master and overwrote the moodle-scalable-cluster-ubuntu directory with the changed content that were all reviewed in this PR. So it'd be greatly appreciated if this PR gets merged once the Travis CI passes. Thanks.

…ta.json

bmoore-msft · 2018-06-28T22:54:11Z

moodle-scalable-cluster-ubuntu/azuredeploy.json

+            "azureBackupSwitch": "[parameters('azureBackupSwitch')]",
+            "azureSearchHostingMode": "[parameters('azureSearchHostingMode')]",
+            "azureSearchName": "[concat('azure-search-',variables('resourceprefix'))]",
+            "azureSearchNameHost": "[concat('azure-search-',variables('resourceprefix'),'.search.windows.net')]",


use reference() to get the uri

bmoore-msft · 2018-06-28T22:54:40Z

moodle-scalable-cluster-ubuntu/azuredeploy.json

+            "ctlrPipName": "[concat('controller-pubip-',variables('resourceprefix'))]",
+            "ctlrVmName": "[concat('controller-vm-',variables('resourceprefix'))]",
+            "ctlrVmSecrets": "[take(variables('ctlrVmSecretsArray'), if(empty(parameters('keyVaultResourceId')), 0, 1))]",
+            "lbDns": "[concat('lb-',variables('resourceprefix'),'.',parameters('location'),'.cloudapp.azure.com')]",


use reference to get the uri

bmoore-msft · 2018-06-28T22:55:18Z

moodle-scalable-cluster-ubuntu/azuredeploy.json

+            "postgresVersion": "[parameters('postgresVersion')]",
+            "redisCacheName": "[concat('redis-',variables('resourceprefix'))]",
+            "redisDeploySwitch": "[parameters('redisDeploySwitch')]",
+            "redisDns": "[concat('redis-',variables('resourceprefix'),'.redis.cache.windows.net')]",


use reference to get the uri

bmoore-msft · 2018-06-28T22:55:34Z

moodle-scalable-cluster-ubuntu/azuredeploy.json

+            "resourcesPrefix": "[variables('resourceprefix')]",
+            "searchType": "[parameters('searchType')]",
+            "serverName": "[concat(parameters('dbServerType'), '-',variables('resourceprefix'))]",
+            "siteURL": "[if(or(empty(parameters('siteURL')), equals(parameters('siteURL'), 'www.example.org')), concat(if(equals(parameters('httpsTermination'), 'AppGw'),'appgw-','lb-'),variables('resourceprefix'),'.',parameters('location'),'.cloudapp.azure.com'), parameters('siteURL'))]",


use reference to get the uri

bmoore-msft · 2018-06-28T22:56:56Z

moodle-scalable-cluster-ubuntu/azuredeploy.parameters.json

-                "vNetAddressSpace":            { "value": "172.31.0.0" },
-                "sshPublicKey":                { "value": "GEN-SSH-PUB-KEY" }
+                "sshPublicKey":                { "value": "GEN-SSH-PUB-KEY" },
+                "dbServerType":                { "value": "mssql" },


remove non-GEN values and use defaultValues in the template

hosungsmsft mentioned this pull request Apr 5, 2018

Implement CI/CD process Azure/Moodle#72

Closed

bmoore-msft requested changes Apr 16, 2018

View reviewed changes

[Moodle] Take in changes from github Azure/Moodle repo

18ab11b

hosungsmsft force-pushed the master branch from a9da1e9 to 18ab11b Compare June 22, 2018 00:47

[Moodle] Fix Travis CI on missing requires property $schema in metada…

feb68a8

…ta.json

bmoore-msft requested changes Jun 28, 2018

View reviewed changes

Remove defaultValues as required

b8a0a37

hosungsmsft closed this Jun 28, 2018

Take in Azure/Moodle changes to azure-quickstart-templates #4514

Take in Azure/Moodle changes to azure-quickstart-templates #4514

Conversation

hosungsmsft commented Apr 4, 2018

Best Practice Checklist

Changelog

hosungsmsft commented Apr 4, 2018

SorraTheOrc commented Apr 9, 2018

Choose a reason for hiding this comment

SorraTheOrc Apr 17, 2018 • edited Loading

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

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

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

Choose a reason for hiding this comment

bmoore-msft commented May 22, 2018

hosungsmsft commented May 22, 2018

hosungsmsft commented May 23, 2018

hosungsmsft commented Jun 22, 2018

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

SorraTheOrc Apr 17, 2018 •

edited

Loading