Using Python for CI script and added smoke test after deployment. #91

roperto · 2018-04-30T01:45:51Z

For more information see Issue #77

msftclas · 2018-04-30T01:46:02Z

All CLA requirements met.

roperto · 2018-04-30T01:51:16Z

This pull request changes the CI to use Azure Python SDK and adds a smoke test (check HTTP status) for the newly created Moodle instance.

Please note I am not sending in Python the ARTIFACTS_LOCATION during the build, do you think that may be a problem?

Also, using Azure CLI it was not necessary to provide a subscription ID, is it something you think the python script should try to detect instead of requesting as a parameter?

As a final note, this still doesn't address issue #90 , but I believe this is an easy-pick to fix if you want it next.

I'll wait for your feedback,

Daniel

hosungsmsft

Hi @roperto

Thanks for your PR. Please see my comments. Not sure about ARTIFACTS_LOCATION, but the _artifactsLocation parameter must be properly composed and overridden with the correct fork name and the branch name. Otherwise, all the nested templates will be pulled from the Azure/Moodle:master, not from the PR or the commit, so the test deployment will be inconsistent. Please see the original bash script about how to get the correct URL for _artifactsLocation (something like SLUG_NAME).

hosungsmsft · 2018-04-30T16:22:06Z

etc/travis/DeploymentTester.py

+            print("*** DEPLOY FAILED ***")
+            print('HTTP Status Code: ' + status)
+            sys.exit(1)
+        print('(ok: {})'.format(status))


Can we try just one more here -- Login to the site as the admin user and see if works?

If I perform a login as admin, it would be interesting to go to the notifications and check if there is any error detected by Moodle as well. I'll see if its reasonable to do that and ping you back.

For now I could probably do it only with curl. Are we planning to add further tests later -- would it have any value to use selenium in the future?

Probably not much to add further tests in the future, but I wanted to make sure at this point that the database is also correctly installed/deployed/configured. A successful Moodle admin login will show that to us, so that's why I'm asking. Using curl should be just fine. We won't be using a too like selenium in the future, at least no plan currently.

A get in the index by itself does test the DB conectivity and many subsystems of Moodle because they are required even for a simple "login page" rendering (there are authentication settings for example stored in the database).

In any case, I implemented a CURL (with follow redirects) to ensure Admin could login and gets to its front page, that gives some extra assertions that the framework is not corrupt. See https://github.com/Azure/Moodle/pull/91/files#diff-0800a30435a8e602ff7bf8df45fc6e09R131

Great to know that hitting the index.php does perform important operations. Thanks for your education. Also thanks much for your admin login check as well!

hosungsmsft · 2018-04-30T16:32:09Z

etc/travis/DeploymentTester.py

+            secret=self.config.secret,
+            tenant=self.config.tenant_id,
+        )
+        self.resource_client = ResourceManagementClient(self.credentials, self.config.subscription_id)


So this seems to be the only place where the subscription ID is needed here, just because of this Python SDK API's requirement. However, a service principal is always associated with an Azure resource ID, which always includes a subscription, so that's why we didn't need to specify a subscription ID in the bash script. I wonder if we can somehow avoid having to provide a subscription ID here? Not a big deal, but just wondering...

I am not very familiar with the the Azure API in Python, but I'd guess it is possible to find out somehow. I'll investigate.

Just mentioning again that this is not a must. If it takes more than half an hour of your time, please disregard this comment.

hosungsmsft · 2018-04-30T16:33:57Z

etc/travis/Configuration.py

+        with open('azuredeploy.parameters.json', 'r') as parameters_fd:
+            parameters = json.load(parameters_fd)
+        parameters = parameters['parameters']
+        parameters['sshPublicKey']['value'] = self.ssh_key


We do need to override the _artifactsLocation parameter here with the correct fork name and the branch name. Otherwise, only the top-level template file (azuredeploy.json) will be from this PR/commit, and all the nested templates will be pulled from Azure/Moodle:master, which is wrong.

My apologies for not understanding the reason for that parameter before, that's the main reason why I asked why we need it. I'll fix it.

That's OK. I should also probably have let you keep using bash, not rewriting the same things in Python anyway. Your fix looks good.

There was a minor change to the logic, but the idea stays the same

hosungsmsft · 2018-05-01T21:12:09Z

I'll still wait for your change to the smoke test to include a site login as the Moodle admin.

roperto · 2018-05-03T03:47:37Z

etc/travis/DeploymentTester.py

+        print('(got credentials)')
+        subscription_client = SubscriptionClient(self.credentials)
+        subscription = next(subscription_client.subscriptions.list())
+        print('(found subscription)')


@hosungsmsft These lines should do the trick to fetch the first subscription (most likely the only one) available for a given service principal so I removed the configurable option.

Excellent! Thanks much for your research and improvement!

roperto · 2018-05-03T05:54:16Z

Hi @hosungsmsft

I have now implemented:

artifacts location fix
auto-detect subscription id
login in Moodle as administrator

Please let me know your thoughts.

Cheers

hosungsmsft · 2018-05-03T15:32:17Z

etc/travis/DeploymentTester.py

+        print("(valid)")
+
+    def deploy(self):
+        print('\nDeploying template, feel free to take a nap...')


hosungsmsft

💯

hosungsmsft · 2018-05-03T15:43:39Z

Fixes #77

roperto added 4 commits April 30, 2018 11:40

Python creates the resource group.

872cbdd

Validate template.

88559e4

Test deploying build.

71b9fc1

Added smoke test for Moodle.

b8dcfba

hosungsmsft suggested changes Apr 30, 2018

View reviewed changes

roperto force-pushed the issue77_moodle-deploy-test branch from 25336f2 to 3668961 Compare May 3, 2018 01:24

Identify "_artifactsLocation" and send as parameter.

f190ec8

roperto force-pushed the issue77_moodle-deploy-test branch from 3668961 to f190ec8 Compare May 3, 2018 01:30

Removed need for subscription id.

23f7665

roperto commented May 3, 2018

View reviewed changes

Add test to login into Moodle as admin

0ef55e9

hosungsmsft reviewed May 3, 2018

View reviewed changes

etc/travis/DeploymentTester.py

print("(valid)")

def deploy(self):

print('\nDeploying template, feel free to take a nap...')

Copy link

hosungsmsft May 3, 2018

Choose a reason for hiding this comment

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

Funny! :)

hosungsmsft approved these changes May 3, 2018

View reviewed changes

hosungsmsft merged commit 02b96ef into Azure:master May 3, 2018

hosungsmsft mentioned this pull request May 3, 2018

Add testing of deployed Moodle site to the CI process #77

Closed

roperto deleted the issue77_moodle-deploy-test branch May 4, 2018 05:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Using Python for CI script and added smoke test after deployment. #91

Using Python for CI script and added smoke test after deployment. #91

roperto commented Apr 30, 2018

msftclas commented Apr 30, 2018 •

edited

Loading

roperto commented Apr 30, 2018 •

edited

Loading

hosungsmsft left a comment

hosungsmsft Apr 30, 2018

roperto May 1, 2018

hosungsmsft May 1, 2018

roperto May 3, 2018

hosungsmsft May 3, 2018

hosungsmsft Apr 30, 2018

roperto May 1, 2018

hosungsmsft May 1, 2018

hosungsmsft Apr 30, 2018

roperto May 1, 2018

hosungsmsft May 1, 2018

roperto May 3, 2018

hosungsmsft commented May 1, 2018

roperto May 3, 2018

hosungsmsft May 3, 2018

roperto commented May 3, 2018

hosungsmsft May 3, 2018

hosungsmsft left a comment

hosungsmsft commented May 3, 2018

Using Python for CI script and added smoke test after deployment. #91

Using Python for CI script and added smoke test after deployment. #91

Conversation

roperto commented Apr 30, 2018

msftclas commented Apr 30, 2018 • edited Loading

roperto commented Apr 30, 2018 • edited Loading

hosungsmsft 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

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

hosungsmsft commented May 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roperto commented May 3, 2018

Choose a reason for hiding this comment

hosungsmsft left a comment

Choose a reason for hiding this comment

hosungsmsft commented May 3, 2018

msftclas commented Apr 30, 2018 •

edited

Loading

roperto commented Apr 30, 2018 •

edited

Loading