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

Projects
None yet
5 participants
@pratikgadiya12
Copy link
Contributor

commented Jul 1, 2019

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

@pratikgadiya12 pratikgadiya12 force-pushed the pratikgadiya12:issue_58370 branch 2 times, most recently from 4849a52 to 07d59b0 Jul 1, 2019

@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 pratikgadiya12:issue_58370 branch 2 times, most recently from f79c2f2 to 9e16c4a Jul 2, 2019

@pratikgadiya12 pratikgadiya12 force-pushed the pratikgadiya12:issue_58370 branch from 9e16c4a to 7dfdef0 Jul 2, 2019

@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 and removed WIP labels Jul 2, 2019

@felixfontein
Copy link
Contributor

left a comment

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

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 lib/ansible/modules/database/mysql/mysql_db.py
Show resolved Hide resolved lib/ansible/modules/database/mysql/mysql_db.py Outdated
Show resolved Hide resolved test/integration/targets/mysql_db/tasks/main.yml

@ansibot ansibot removed the needs_triage label Jul 2, 2019

@Andersson007
Copy link
Contributor

left a comment

@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

@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 label Jul 3, 2019

pratikgadiya12 added some commits Jul 16, 2019

Refactored tests
- Added removal of dump2 file
@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2019

@pratikgadiya12

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2019

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 label Jul 17, 2019

@Andersson007
Copy link
Contributor

left a comment

shipit

@Andersson007

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

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

@pratikgadiya12

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

!needs_revision

@ansibot ansibot added core_review and removed needs_revision labels Jul 18, 2019

@bmalynovytch

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

bot_status

@ansibot

This comment has been minimized.

Copy link
Contributor

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 label Jul 18, 2019

@bmalynovytch

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

shipit

@ansibot ansibot added shipit and removed needs_revision labels Jul 18, 2019

@felixfontein felixfontein merged commit 393e4a4 into ansible:devel Jul 18, 2019

1 check passed

Shippable Run 132459 status is SUCCESS.
Details
@felixfontein

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.