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.

Sign up for GitHub

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

Jump to bottom

[new module] Added Azure SQL Servers support - cloud/azure/azure_rm_sql_servers #33077

Merged

jborean93 merged 17 commits into ansible:devel from VSChina:module-azure-rm-sql-servers

Dec 13, 2017

Contributor

zikalino commented Nov 20, 2017

SUMMARY

Adding support for Azure SQL Servers

ISSUE TYPE

New Module Pull Request

COMPONENT NAME

azure_rm_sql_servers

ANSIBLE VERSION

ansible 2.5.0 (devel 125c4b135d) last updated 2017/10/17 07:30:02 (GMT +000)
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/root/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /ansible/lib/ansible
  executable location = /ansible/bin/ansible
  python version = 2.7.12 (default, Nov 19 2016, 06:48:10) [GCC 5.4.0 20160609]

ADDITIONAL INFORMATION


          Added new module: Azure RM SQL Servers

904999c

Contributor

ansibot commented Nov 20, 2017

cc @brusMX
click here for bot help

ansibot added affects_2.5 azure cloud community_review module needs_triage new_module new_plugin support:community test labels

Contributor

ansibot commented Nov 20, 2017

The test ansible-test sanity --test pep8 [?] failed with the following error:

lib/ansible/modules/cloud/azure/azure_rm_sql_servers.py:48:161: E501 line too long (209 > 160 characters)

The test ansible-test sanity --test validate-modules [?] failed with the following errors:

lib/ansible/modules/cloud/azure/azure_rm_sql_servers.py:0:0: E305 DOCUMENTATION.options.identity.suboptions.type.description.0: expected str @ data['options']['identity']['suboptions']['type']['description'][0]. Got {"The identity type. Set this to 'SystemAssigned' in order to automatically create and assign an Azure Active Directory principal for the resource. Possible values include": 'SystemAssigned'}
lib/ansible/modules/cloud/azure/azure_rm_sql_servers.py:0:0: E312 No RETURN provided

click here for bot help

ansibot added ci_verified needs_revision and removed community_review labels

bcoca removed the needs_triage label


          fixed sanity issues

295dac5

ansibot removed the ci_verified label

Contributor

ansibot commented Nov 22, 2017

@brusMX @devigned @haroldwongms @julienstroheker @jwhitbeck @lmazuel @obsoleted @ozboms @sozercan @tstringer @xscript @yuwzho

As a maintainer of a module in the same namespace this new module has been submitted to, your vote counts for shipits. Please review this module and add shipit if you would like to see it merged.

click here for bot help

ansibot added community_review and removed needs_revision labels


          enabled integration test

71ca531

ansibot added needs_revision and removed community_review labels


          disabling test as Microsoft.Sql namespace not available

a9bee14

ansibot added community_review and removed needs_revision labels


          renamed servers -> server

52e4a8b

jborean93 requested changes

View reviewed changes

Contributor

jborean93 left a comment

Have added some comments, some things we would need to do before merging is

Fix up the idempotency, right now it reports a change everytime it runs
Expand the tests to validate the module
Make the return values more Ansible like

Overall the module works for me and creates a SQL Server which is great. One question I do have more on the Azure side, is it planned to add support for more SQL types, e.g. MySQL and so or is that not supported in Azure or going to be in separate modules?

lib/ansible/modules/cloud/azure/azure_rm_sql_server.py Outdated

+              ---
+              module: azure_rm_sql_server
+              version_added: "2.5"
+              short_description: Manage an Server.

Contributor

jborean93 Nov 23, 2017

short_description should not have a ., are you able to expand on this like, Manage a Azure SQL Server?

lib/ansible/modules/cloud/azure/azure_rm_sql_server.py Outdated

+              version_added: "2.5"
+              short_description: Manage an Server.
+              description:
+                  - Create, update and delete an instance of Server.

Contributor

jborean93 Nov 23, 2017

SQL

lib/ansible/modules/cloud/azure/azure_rm_sql_server.py Outdated

+                  location:
+                      description:
+                          - Resource location.
+                      required: True

Contributor

jborean93 Nov 23, 2017

It would be nice if this wasn't required and could be derived from the resource group specified.

lib/ansible/modules/cloud/azure/azure_rm_sql_server.py Outdated

+                  tags:
+                      description:
+                          - Resource tags.
+                      required: False

Contributor

jborean93 Nov 23, 2017

required: False is the default and is usually omitted.

lib/ansible/modules/cloud/azure/azure_rm_sql_server.py Outdated

+                      suboptions:
+                          type:
+                              description:
+                                  - "The identity type. Set this to 'SystemAssigned' in order to automatically create and assign an Azure Active Directory principal for th

Contributor

jborean93 Nov 24, 2017

I would argue that SystemAssigned should be the default value if this is not set or can a SQL Server not have an identity? Is there a reason why this is a nested option under the type key? If not can identity just by the type value or do you see there being more options available to identity?

When quoting a value, you can enclose it with C(), e.g. 'SystemAssigned' should be C(SystemAssigned). You can also use I() when referring to option option names.

lib/ansible/modules/cloud/azure/azure_rm_sql_server.py Outdated

+                  def exec_module(self, **kwargs):
+                      """Main module execution method"""
+                      for key in list(self.module_arg_spec.keys()) + ['tags']:

Contributor

jborean93 Nov 24, 2017

Have a look at azure_rm_virtualmachine to see how to do this, you usually set the parameter as None in the init and these are populated in AzureRMModuleBase for you.

lib/ansible/modules/cloud/azure/azure_rm_sql_server.py Outdated

+                      else:
+                          self.log("Server instance already exists")
+                          if self.state == 'absent':
+                              self.delete_servers()

Contributor

jborean93 Nov 24, 2017

This shouldn't be run in check mode.

lib/ansible/modules/cloud/azure/azure_rm_sql_server.py Outdated

+                              self.results['changed'] = True
+                              self.log("Server instance deleted")
+                          elif self.state == 'present':
+                              self.log("Need to check if Server instance has to be deleted or may be updated")

Contributor

jborean93 Nov 24, 2017

You need some code to check if the server actually needs updating, right now the module is reporting a change when running it twice which is not idempotent.

lib/ansible/modules/cloud/azure/azure_rm_sql_server.py Outdated

+                              self.results['state'] = self.create_update_servers()
+                              self.results['changed'] = True
+                          else:
+                              self.results['state'] = response

Contributor

jborean93 Nov 24, 2017

We should look at changing the return values, see my note in the documentation portion.

test/integration/targets/azure_rm_sql_server/tasks/main.yml Outdated

		@@ -0,0 +1,13 @@
		- name: Create Servers

Contributor

jborean93 Nov 24, 2017

I know you were saying you were having issues trying to get this to run but we should also look at expanding these tests a lot more, usually we look at performing 3 tasks per action, e.g. 3 tasks for creating a server, 3 tasks for modifying a server and 3 tasks for removing a server. The 3 tasks per action should follow;

Run the task in check mode and check that the server wasn't actually created
Run the task normally and check that the server was created as expected
Run the task again (idempotent) and check that a change wasn't reported

Contributor Author

zikalino commented Dec 12, 2017

ready_for_review

Contributor Author

zikalino commented Dec 12, 2017

@jborean93 please check the module again, i think it's ready for merging.

jborean93 requested changes

View reviewed changes

Contributor

jborean93 left a comment •

edited

I couldn't really test this as I came across the error

fatal: [127.0.0.1]: FAILED! => {"changed": false, "msg": "Error creating the SQL Server instance: Azure Error: InvalidRequestContent\nMessage: The request content was invalid and could not be deserialized: 'Required property 'type' not found in JSON. Path 'identity', line 1, position 44.'."}

A few things to note;

The tests need to be activated by adding the alias I mentioned in the comment (that should have picked up the error I recieved)
The tests need to be expanded to cover a check mode, normal and idempotent tests of the actions create, update, and delete scenarios
I'm not sure if the tags are actually supported, I was unable to test it but couldn't find anything that really seem to check and set them
The location is still required, it fails if you don't set it

lib/ansible/modules/cloud/azure/azure_rm_sqlserver.py Outdated

+                      description:
+                          - The name of the server.
+                      required: True
+                  tags:

Contributor

jborean93 Dec 12, 2017

I'm pretty sure you don't need the tags as you are extending the azure_tags documentation below.

lib/ansible/modules/cloud/azure/azure_rm_sqlserver.py

+                  def exec_module(self, **kwargs):
+                      """Main module execution method"""
+                      for key in list(self.module_arg_spec.keys()) + ['tags']:

Contributor

jborean93 Dec 12, 2017

What's the point of this whole block, AzureRMModuleBase should be setting self.x based on the module_arg_spec

Contributor Author

zikalino Dec 12, 2017

well, i have tried and it doesn't work as expected, so i have to leave it for now...

Contributor

jborean93 Dec 12, 2017

Ok, will try and debug and find out what is happening, it should just work.

Contributor

jborean93 Dec 12, 2017

Ok I see my mistake, most modules just have

for key in list(self.module_arg_spec.keys()) + ['tags']:
    setattr(self, key, kwargs[key])

Is there a reason why you have all these if statements as the above should be enough for you.

Contributor Author

zikalino Dec 12, 2017

yes, these attributes are sticked into objects padded to Azure SDK, so treated in different way

lib/ansible/modules/cloud/azure/azure_rm_sqlserver.py Outdated

+                      self.mgmt_client = self.get_mgmt_svc_client(SqlManagementClient,
+                                                                  base_url=self._cloud_environment.endpoints.resource_manager)
+                      try:

Contributor

jborean93 Dec 12, 2017

No need for a try/except here as azure_rm_common handles a missing source group

lib/ansible/modules/cloud/azure/azure_rm_sqlserver.py Outdated

+                      except CloudError:
+                          self.fail('resource group {0} not found'.format(self.resource_group))
+                      if not ("location" in self.parameters):

Contributor

jborean93 Dec 12, 2017

This doesn't seem to be working, I got The error was: msrest.exceptions.ValidationError: Parameter 'Server.location' can not be None in an error which means this musn't work. Try doing;

if self.location is None:
    self.location = resource_group.location

lib/ansible/modules/cloud/azure/azure_rm_sqlserver.py Outdated

+                      self.parameters = dict()
+                      self.parameters['identity'] = dict()
+                      self.results = dict(changed=False, state=dict())

Contributor

jborean93 Dec 12, 2017

state doesn't need to be there.

lib/ansible/modules/cloud/azure/azure_rm_sqlserver.py Outdated

+                              self.results['changed'] = True
+                          else:
+                              self.results['changed'] = old_response.__ne__(response)
+                          self.results.update(response)

Contributor

jborean93 Dec 12, 2017

Instead of updating the results object here with the response and then removing the unneeded ones below, just set the results you want manually.

lib/ansible/modules/cloud/azure/azure_rm_sqlserver.py Outdated

+                          self.log("Creation / Update done")
+                      elif self.to_do == Actions.Delete:
+                          self.log("SQL Server instance deleted")
+                          self.delete_sqlserver()

Contributor

jborean93 Dec 12, 2017

Need to not run when in check mode

lib/ansible/modules/cloud/azure/azure_rm_sqlserver.py Outdated

+                          self.results['changed'] = True
+                      else:
+                          self.log("SQL Server instance unchanged")
+                          self.results['state'] = old_response

Contributor

jborean93 Dec 12, 2017

This shouldn't be returned, it should be the same values that you set in add or update.

test/integration/targets/azure_rm_sqlserver/aliases

		@@ -0,0 +1,2 @@
		cloud/azure
		destructive

Contributor

jborean93 Dec 12, 2017

You will need to add posix/ci/cloud/group2/azure to this list so the tests actually run in CI

ansibot added needs_revision and removed community_review labels


          updates as requested in the review

3ce1514

Contributor Author

zikalino commented Dec 12, 2017

@jborean93 I have made all necessary updates, seems like test can't run as Microsoft.Sql namespace has to be added.

Error creating the SQL Server instance: Azure Error: MissingSubscriptionRegistration\nMessage: The subscription is not registered to use namespace 'Microsoft.Sql'. See https://aka.ms/rps-not-found for how to register subscriptions.\nException Details:\n\tError Code: MissingSubscriptionRegistration\n\tMessage: The subscription is not registered to use namespace 'Microsoft.Sql'. See https://aka.ms/rps-not-found for how to register subscriptions.\n\tTarget: Microsoft.Sql

Contributor

jborean93 commented Dec 12, 2017 •

edited

Looks like the tests are failing because we aren't authorised in CI, will see if @mattclay can help.

Edit: just saw your post :)

Contributor

jborean93 commented Dec 12, 2017 •

edited

@zikalino once we get the tests running we need to look at expanding the tests, there is at least 1 issue I can find that the tests should have picked up if we had some more tests. Let me know if you are confused as to what I am asking for, I'm happy to discuss it further over IRC as it would be good to get this merged today so you can update your other PR's with the changes you made here.

Contributor Author

zikalino commented Dec 12, 2017

@jborean do you mean taking location from resource group? this could be difficult. some resources can't be created in particular location (I believe i couldn't create SQL server in the same location as the one where default rg is created).

Contributor

jborean93 commented Dec 12, 2017

@zikalino, for the location part I see the benefit in being able to set the location independently to override the resource group but most modules seem to have it as an optional value and if not set explicitly it is derived from the resource group location.

What I am talking about is expanding the tests from just the create, update, delete tasks to test each scenario under check mode, normal and an idempotent task. In the end it would look similar to

- name: create sql server (check)
  azure_rm_sqlserver:
    ...
  register: create_server_check

- name: check if server exists (check)
  ... ? use a facts module if possible
  register: create_server_result_check

- name: assert create sql server results (check)
  assert:
    that:
    - create_server_check is changed
    - create_server_check.exists == False

- name: create sql server
  azure_rm_sqlserver:
    ...
  register: create_server

- name: check if server exists
  ... ? use a facts module if possible
  register: create_server_result

- name: assert create sql server results
  assert:
    that:
    - create_server is changed
    - create_server.exists == True

- name: create sql server (idempotent)
  azure_rm_sqlserver:
    ...
  register: create_server_again

- name: assert create sql server results (idempotent)
  assert:
    that:
    - not create_server_again is changed

This would be repeated again for updating a SQL server and then finally again for removing a SQL server.

Contributor

jborean93 commented Dec 12, 2017

@zikalino, the Microsoft.SQL resource has been registered and the tests are now running.

Contributor Author

zikalino commented Dec 12, 2017

@jborean93 seems now tests are passing. should i still add more tests to this pr? or is it ok to merge, and i could expand tests when adding facts module?

Contributor

jborean93 commented Dec 12, 2017 •

edited

I think we should still continue to add the extra tests just without that second step of checking the results with a fact module for now (should still come back and expand once a facts module is in). We can still do some assertions on the results of just the task itself.

Do you know how to run tests locally?

zikalino added 2 commits

December 13, 2017 09:50


          added more tests

8d779ac


          deleting unexisting instance up front

409c981

Contributor Author

zikalino commented Dec 13, 2017

@jborean93 I have more tests for check_mode and also delete unexisting sql server test (which i moved to front, as delete after delete seems a bit unstable -- second delete may get invalid information that resource still exists)

Contributor

jborean93 commented Dec 13, 2017

Looks better, we still need a test for each action again to ensure that no changes are recorded (idempotent).


          more tests

c131a84

Contributor Author

zikalino commented Dec 13, 2017

@jborean93 how about now? added 2 more tests

jborean93 reviewed

View reviewed changes

Contributor

jborean93 left a comment

Looking good, left some comments on some tests, only thing remaining is to delete the instance again right at the end for idempotency checks

test/integration/targets/azure_rm_sqlserver/tasks/main.yml Outdated

+                    - output.changed
+                    - output.state == 'Ready'
+              - name: Create again instance of SQL Server -- check mode

Contributor

jborean93 Dec 13, 2017 •

edited

Delete this one, you only need to run the check mode test for a scenario once

test/integration/targets/azure_rm_sqlserver/tasks/main.yml Outdated

+                  random_postfix: "{{ 1000 | random }}"
+                run_once: yes
+              - name: Delete unexisting instance of SQL Server -- check mode

Contributor

jborean93 Dec 13, 2017

Take away this test

test/integration/targets/azure_rm_sqlserver/tasks/main.yml Outdated

+                  that:
+                    - output.changed == false
+              - name: Delete unexisting instance of SQL Server

Contributor

jborean93 Dec 13, 2017

Don't need this test as you should be starting with a random resource group so no SQL server would exist anyway

zikalino and others added 2 commits

December 13, 2017 11:38


          another round of test changes

fffd0d6


          removed uneeded test

e023063

jborean93 approved these changes

View reviewed changes

Contributor Author

zikalino commented Dec 13, 2017

shipit

jborean93 merged commit eb5ffa0 into ansible:devel

Contributor

jborean93 commented Dec 13, 2017

Awesome @zikalino, are you able to raise a PR to add this module to the changelog for 2.5 https://github.com/ansible/ansible/blob/devel/CHANGELOG.md. We try to keep things in alphabetical order in each section.

zikalino deleted the module-azure-rm-sql-servers branch

December 13, 2017 04:31

ansible locked and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

affects_2.5 azure cloud module needs_revision new_module new_plugin support:community test