[new module] Adding module azure_rm_sqldatabase #33173
Conversation
ansibot
added
affects_2.5
|
@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 |
gundalow
removed
the
needs_triage
|
@zikalino this PR contains more than one new module. Please submit only one new module per pullrequest. For further explanation, please read grouped module documentation |
ansibot
added
needs_revision
There was a problem hiding this comment.
I haven't tested this one yet but a lot of the comments I had for your azure_rm_sql_server also apply this this one here when it comes to the code. One area I think this needs to be improved on is the documentation. I started putting a few comments but they mostly apply to all the options and it all balls down to;
- Don't copy and paste from docs as the option names don't match up to the Ansible side
- Put the text after a
.on a new line like in the examples I gave you - When talking about behaviour for different options, try and put them on their own sentance.
- Expand the examples as there seems to be quite a few different ways of doing things
We should also look at changing the return values to only return what the user may not already know but we can look at that after we get the documentation updated and the tests expanded.
| @@ -0,0 +1,443 @@ | |||
| #!/usr/bin/python | |||
There was a problem hiding this comment.
Seems like you copied the original but didn't remove it :)
| --- | ||
| module: azure_rm_sql_database | ||
| version_added: "2.5" | ||
| short_description: Manage an Databases. |
There was a problem hiding this comment.
We don't put . on the short_description.
| location: | ||
| description: | ||
| - Resource location. | ||
| required: True |
There was a problem hiding this comment.
Like with azure_sql_server, can we not require this and derive the location from the resource group if not specified?
| required: False | ||
| create_mode: | ||
| description: | ||
| - "Specifies the mode of database creation.\n\nDefault: regular database creation.\n\nCopy: creates a database as a copy of an existing database. |
There was a problem hiding this comment.
Let's not copy and paste the values from the documentation and do something like.
create_mode:
description:
- Specifies the mode of the database creation.
- C(Default) is a regular database creation
- C(Copy) creates a database as a copy of an existing database, I(source_database_id) must be set in this mode.
- C(OnlineSecondary) creates a database as a readable secondary replica, I(source_database_id) must be set in this mode.
- ... continue for each option and state what needs to be set
default: Default
| ecovery point resource ID.\n\nCopy, NonReadableSecondary, OnlineSecondary and RestoreLongTermRetentionBackup are not supported for DataWarehou | ||
| se edition. Possible values include: 'Copy', 'Default', 'NonReadableSecondary', 'OnlineSecondary', 'PointInTimeRestore', 'Recovery', 'Restore' | ||
| , 'RestoreLongTermRetentionBackup'" | ||
| required: False |
There was a problem hiding this comment.
required: False not needed.
| required: False | ||
| source_database_id: | ||
| description: | ||
| - "Conditional. If createMode is Copy, NonReadableSecondary, OnlineSecondary, PointInTimeRestore, Recovery, or Restore, then this value is requir |
There was a problem hiding this comment.
Split up as an entry per sentence instead of one long line, e.g.
source_database_id:
description:
- Specifies the resource ID of the source database.
- If I(create_mode) is C(Copy), C(NonReadableSecondary), ...., then this must be set.
- If I(create_mode) is C(NonReadableSecondary) or C(OnlineSecondary), the name of the source db must be the same as the new db.
Maybe even make it clearer whether this is the name of the database or the id, or both?
| ''' | ||
|
|
||
| EXAMPLES = ''' | ||
| - name: Create (or update) Sql |
There was a problem hiding this comment.
A few things
- Keep the examples left aligned (remove the leading 4 spaces)
- Stop using jinja2 block
{{}}in the examples so people can see what the actual may look like - Provide more than 1 example, usually we have a basic with only the parameters required, then 1 for each major scenario, like 1 for the main create_mode options and so on
|
The test The test |
|
One more name change as well, the common format is to use |
ansibot
removed
the
ci_verified
ansibot
added
the
needs_ci
|
The test |
ansibot
added
ci_verified
|
ready_for_review |
ansibot
added
community_review
|
fixed several major issues:
Additional note - I still plan to add following adjustments in this PR:
|
ansibot
added
stale_ci
nitzmahone
changed the title
| @@ -0,0 +1,2 @@ | |||
| cloud/azure | |||
There was a problem hiding this comment.
Are you able to update this so it runs on the CI
| location: eastus | ||
| admin_username: mylogin | ||
| admin_password: Testpasswordxyz12! | ||
| - name: Create instance of SQL Database |
There was a problem hiding this comment.
Are you able to add a check mode and idempotent task for the create, update and delete tasks
| version_added: "2.5" | ||
| short_description: Manage SQL Database instance | ||
| description: | ||
| - Create, update and delete instance of SQL Database |
| description: | ||
| - The name of the database to be operated on (updated or created). | ||
| required: True | ||
| tags: |
There was a problem hiding this comment.
The extends documentation fragments azure_tags adds this option
| - Resource tags. | ||
| location: | ||
| description: | ||
| - Resource location. |
There was a problem hiding this comment.
What is the default value if not set, from the resource group?
| self.log("Checking if the SQL Database instance {0} is present".format(self.name)) | ||
| found = False | ||
| try: | ||
| response = self.mgmt_client.databases.get(self.resource_group, |
| self.log("Need to Create / Update the SQL Database instance") | ||
|
|
||
| if self.check_mode: | ||
| return self.results |
There was a problem hiding this comment.
This would need results['changed'] = True in the create scenario. We really need a way to checking if we need to update the database instead of doing the operation and checking if it changed there as well.
| if not old_response: | ||
| self.results['changed'] = True | ||
| else: | ||
| self.results['changed'] = old_response.__ne__(response) |
There was a problem hiding this comment.
This isn't really how we usually go update operations in Ansible. Is there way of checking if a change needs to occur and then making or rather than making a request and seeing if it was changed. The benefit of the former approach is that when running in check mode you can see if a change needs to happen while in this scenario you wouldn't know.
| self.results['changed'] = True | ||
| else: | ||
| self.results['changed'] = old_response.__ne__(response) | ||
| self.results.update(response) |
There was a problem hiding this comment.
Don't do this as you would need to pop all the results below. Just set results['id'] = response.... for each value you want to return to make it simpler.
| assert: | ||
| that: | ||
| - output.changed | ||
| - output.status == 'Online' |
There was a problem hiding this comment.
Would be good to check that some of the return values like id is defined, like so
- name: Assert the resource instance is created
assert:
that:
- output.changed
- output.status == 'Online'
- output.id is defined
- output.database_id is defined
SUMMARY
Adding support for Azure SQL Databases
ISSUE TYPE
COMPONENT NAME
azure_rm_sql_databases
ANSIBLE VERSION
ADDITIONAL INFORMATION
This module depends on azure_rm_sql_servers