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

Allow multiple databases(not all) to be dumped from mysql #56721

Merged
merged 3 commits into from Jun 24, 2019

Conversation

@pratikgadiya12
Copy link
Contributor

commented May 21, 2019

SUMMARY

Fixes: #56059

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

mysql

@ansibot

This comment has been minimized.

@mattclay

This comment has been minimized.

Copy link
Member

commented May 22, 2019

bot_status

@ansibot

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

Components

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

Metadata

waiting_on: pratikgadiya12
changes_requested_by: null
needs_info: False
needs_revision: True
needs_rebase: False
merge_commits: []
too many files or commits: False
mergeable_state: unstable
shippable_status: failure
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 needs_triage label May 22, 2019

@pratikgadiya12 pratikgadiya12 force-pushed the pratikgadiya12:issues_56059 branch from b70027c to 3cdf70e May 22, 2019

@ansibot ansibot removed the ci_verified label May 22, 2019

@bmalynovytch
Copy link
Contributor

left a comment

Giving a list of databases when importing seems a non-sens to me, as the database names should be provided in the SQL file to import.
I also suggest that the name parameter should allow to be either a string or a list, like many other modules accepting multiple values.

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 Outdated
Show resolved Hide resolved lib/ansible/modules/database/mysql/mysql_db.py Outdated
@bmalynovytch

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

Could you also describe the tests you made with the current changes ? (dumps, imports, environments on each, etc ...)

@pratikgadiya12

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2019

I also suggest that the name parameter should allow to be either a string or a list, like many other modules accepting multiple values.

@bmalynovytch To be clear, does this mean that if someone wants to specify multiple databases in mysql dump, then he/she needs to provide it in the form of a list?

Multiple databases: Should be provided in list
Single database: Should be provided in string

@bmalynovytch

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

@pratikgadiya12 Some modules accept multiple forms of arguments:

  • string without coma for single entry (converted internally as list of one item)
  • string with coma for multiple entries (split into list)
  • list

Look at https://docs.ansible.com/ansible/latest/modules/yum_module.html, argument name

@pratikgadiya12 pratikgadiya12 force-pushed the pratikgadiya12:issues_56059 branch from 3cdf70e to c561892 May 24, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

lib/ansible/modules/database/mysql/mysql_db.py:99:5: E311 EXAMPLES is not valid YAML

The test ansible-test sanity --test yamllint [explain] failed with 2 errors:

changelogs/fragments/56059-mysqldb-allow-multiple_databases_in_dump_operation.yml:3:1: empty-lines too many blank lines (1 > 0)
lib/ansible/modules/database/mysql/mysql_db.py:99:5: error EXAMPLES: syntax error: could not find expected ':'

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

@pratikgadiya12 pratikgadiya12 force-pushed the pratikgadiya12:issues_56059 branch from c561892 to ab4d04b May 24, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

The test ansible-test sanity --test yamllint [explain] failed with 1 error:

changelogs/fragments/56059-mysqldb-allow-multiple_databases_in_dump_operation.yml:3:1: empty-lines too many blank lines (1 > 0)

click here for bot help

@pratikgadiya12 pratikgadiya12 force-pushed the pratikgadiya12:issues_56059 branch 3 times, most recently from 01336a6 to 98d1eb6 May 24, 2019

@pratikgadiya12 pratikgadiya12 force-pushed the pratikgadiya12:issues_56059 branch from acb427f to c201d68 Jun 19, 2019

@pratikgadiya12 pratikgadiya12 force-pushed the pratikgadiya12:issues_56059 branch from c201d68 to 9f3c6ad Jun 19, 2019

@ansibot ansibot removed the support:core label Jun 19, 2019

@pratikgadiya12 pratikgadiya12 force-pushed the pratikgadiya12:issues_56059 branch from 9f3c6ad to cdf4be9 Jun 19, 2019

@Andersson007
Copy link
Contributor

left a comment

thank you for working on this!

@Andersson007

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

shipit

@pratikgadiya12 pratikgadiya12 force-pushed the pratikgadiya12:issues_56059 branch from cdf4be9 to e148650 Jun 20, 2019

@Andersson007

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

shipit

@pratikgadiya12

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

@bmalynovytch Could you clear the requested changes since there's nothing left?

We are waiting for one more shipit here to get it merged

@bmalynovytch

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

LGTM

shipit

@felixfontein felixfontein merged commit 44058e9 into ansible:devel Jun 24, 2019

1 check passed

Shippable Run 128754 status is SUCCESS.
Details
@felixfontein

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

@pratikgadiya12 thanks for implementing this!
@bmalynovytch @Andersson007 @samccann thanks for reviewing this!

This new feature will be available in Ansible 2.9.

@Andersson007

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

@pratikgadiya12 , I congratulate you on merging this PR!
It would be better if we could create / drop / etc. several databases in the same way. If you decide to do this, please, let me know before starting :) thank you

@pratikgadiya12

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2019

@pratikgadiya12 , I congratulate you on merging this PR!
It would be better if we could create / drop / etc. several databases in the same way. If you decide to do this, please, let me know before starting :) thank you

Thanks, @Andersson007.
Sure, I wanted to work on the idea mentioned above, however, was waiting for this patch to get merged.
Now, since this has been merged, I will start working on the PR to support create/drop several databases.

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.