Skip to content
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

Added support to create and delete multiple databases in MySQL #58602

Merged
merged 21 commits into from
Jul 18, 2019

Conversation

pratikgadiya12
Copy link
Contributor

SUMMARY

Creation/Deletion of multiple databases now supported in MySQL

Fixes: #58370

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

lib/ansible/modules/database/mysql/mysql_db.py

@ansibot
Copy link
Contributor

ansibot commented Jul 1, 2019

@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. affects_2.9 This issue/PR affects Ansible v2.9 database Database category feature This issue/PR relates to a feature request. module This issue/PR relates to a module. mysql needs_triage Needs a first human triage before being processed. support:community This issue/PR relates to code supported by the Ansible community. test This PR relates to tests. labels Jul 1, 2019
@pratikgadiya12 pratikgadiya12 force-pushed the issue_58370 branch 2 times, most recently from 4849a52 to 07d59b0 Compare July 1, 2019 20:00
@Akasurde Akasurde changed the title [WIP] Added support to create/delete mulitiple databases in MySQL [WIP] Added support to create and delete multiple databases in MySQL Jul 2, 2019
@pratikgadiya12 pratikgadiya12 force-pushed the issue_58370 branch 2 times, most recently from f79c2f2 to 9e16c4a Compare July 2, 2019 10:47
@pratikgadiya12 pratikgadiya12 changed the title [WIP] Added support to create and delete multiple databases in MySQL Added support to create and delete multiple databases in MySQL Jul 2, 2019
@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. labels Jul 2, 2019
Copy link
Contributor

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Please rework the logic using the lists. I'll take another look then.

lib/ansible/modules/database/mysql/mysql_db.py Outdated Show resolved Hide resolved
lib/ansible/modules/database/mysql/mysql_db.py Outdated Show resolved Hide resolved
lib/ansible/modules/database/mysql/mysql_db.py Outdated Show resolved Hide resolved
lib/ansible/modules/database/mysql/mysql_db.py Outdated Show resolved Hide resolved
@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label Jul 2, 2019
Copy link
Contributor

@Andersson007 Andersson007 left a comment

Choose a reason for hiding this comment

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

@pratikgadiya12 , thank you for this PR!
Let's start from covering these changes by integration tests. Then I make code and doc reviews.

  1. First of all, regarding that @felixfontein said , please, remove tests in a comma-separated form or a list form (on your choice) from the test file because they are interchangeable
  2. We should check:
  • Create multiple databases check_mode (done)
  • Create multiple databases (partially done) - you check it by: "'database%' not in mysql_result.stdout" it may return true when one db exists, but we need to be sure that the both databases exist, so, better to check them by names
  • Delete multiple databases check_mode (done)
  • Delete multiple databases (done)
  • Recreate multiple databases (done)

Please add checks for the following case:

  • As I wrote above, when we check that the databases exist on the server instance we need to be sure that each of them exists, change it please (preferably by check them by names).
  • Delete one of the databases, result.changed == true
  • Check that it doesn't exist and the second db exists
  • Try to create both in check_mode, result.changed == true
  • Check that the deleted db actually doesn't exist, the second exist
  • Try to create both again in actual mode, result.changed == true
  • Check that the both exist
  • Remove both databases to clean up

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Jul 3, 2019
@pratikgadiya12 pratikgadiya12 changed the title Added support to create and delete multiple databases in MySQL [WIP] Added support to create and delete multiple databases in MySQL Jul 3, 2019
@ansibot ansibot added the WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. label Jul 3, 2019
@ansibot
Copy link
Contributor

ansibot commented Jul 17, 2019

@ansibot ansibot added docs This issue/PR relates to or includes documentation. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Jul 17, 2019
@pratikgadiya12
Copy link
Contributor Author

thank you, guys, for the great work!
two points about tests:

  1. IMO better to move tests that related to import to another file (suitable existent or new)
  2. there are duplicated tests, @pratikgadiya12 , if you check, for example, deletion, you don't need to do it again each time when you need to delete databases for the next steps, etc. So, better to reduce amount of test code by deleting assertions that were before (you could remark in test comment that you delete databases because it's needed for the next step, etc.)
  3. The module return db/db_list so, IMO better to add RETURN = r'''...''' block (see almost any postgresql modules) and check it into the tests, e.g.:
- assert:
    that:
    - result is changed
    - result.db == [..., ..., ...]

Thanks for the review @felixfontein and @Andersson007!
I have fixed all the above review comments

@pratikgadiya12 pratikgadiya12 changed the title [WIP] Added support to create and delete multiple databases in MySQL Added support to create and delete multiple databases in MySQL Jul 17, 2019
@ansibot ansibot removed the WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. label Jul 17, 2019
Copy link
Contributor

@Andersson007 Andersson007 left a comment

Choose a reason for hiding this comment

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

shipit

@Andersson007
Copy link
Contributor

@pratikgadiya12 thanks for the contribution, great work, thanks!
@felixfontein , thanks for reviewing!

@pratikgadiya12
Copy link
Contributor Author

Thanks a lot @felixfontein and @Andersson007 for great review!

We could merge this @felixfontein since this PR has 2 shipit's from you guys :)

@Andersson007
Copy link
Contributor

!needs_revision

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jul 18, 2019
@bmalynovytch
Copy link
Contributor

bot_status

@ansibot
Copy link
Contributor

ansibot commented Jul 18, 2019

Components

changelogs/fragments/58370-mysqldb-allow-multiple_databases_create_delete_operation.yml
support: community
maintainers:

docs/docsite/rst/porting_guides/porting_guide_2.9.rst
support: core
maintainers: acozine

lib/ansible/modules/database/mysql/mysql_db.py
support: community
maintainers: Alexander198961 Andersson007 Xyon ansible bmalynovytch bmildren michaelcoburn oneiroi tolland

test/integration/targets/mysql_db/tasks/main.yml
support: community
maintainers: Alexander198961 Andersson007 Xyon ansible bmalynovytch bmildren michaelcoburn oneiroi tolland

test/integration/targets/mysql_db/tasks/multi_db_create_delete.yml
support: community
maintainers: Alexander198961 Andersson007 Xyon ansible bmalynovytch bmildren michaelcoburn oneiroi tolland

test/integration/targets/mysql_db/tasks/state_dump_import.yml
support: community
maintainers: Alexander198961 Andersson007 Xyon ansible bmalynovytch bmildren michaelcoburn oneiroi tolland

Metadata

waiting_on: pratikgadiya12
changes_requested_by: bmalynovytch
needs_info: False
needs_revision: True
needs_rebase: False
merge_commits: []
too many files or commits: False
mergeable_state: clean
shippable_status: success
maintainer_shipits (module maintainers): False
community_shipits (namespace maintainers): False
ansible_shipits (core team members): False
shipit_actors (maintainer or core team member): None
shipit_actors_other:
automerge: automerge shipit test failed

click here for bot help

@ansibot ansibot removed the core_review In order to be merged, this PR must follow the core review workflow. label Jul 18, 2019
@bmalynovytch
Copy link
Contributor

shipit

@ansibot ansibot added shipit This PR is ready to be merged by Core and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jul 18, 2019
@felixfontein felixfontein merged commit 393e4a4 into ansible:devel Jul 18, 2019
@felixfontein
Copy link
Contributor

@pratikgadiya12 thanks a lot for implementing this!
@bmalynovytch @Andersson007 thanks a lot for reviewing!

@ansible ansible locked and limited conversation to collaborators Aug 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.9 This issue/PR affects Ansible v2.9 database Database category docs This issue/PR relates to or includes documentation. feature This issue/PR relates to a feature request. module This issue/PR relates to a module. mysql shipit This PR is ready to be merged by Core support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow mysql_db module to create/drop multiple databases
5 participants