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

Adding encoding to import function #61215

Open
wants to merge 7 commits into
base: devel
from

Conversation

@netmonk
Copy link

commented Aug 23, 2019

After defining a latin1 database, there are error when importing sql scripts encoded in latin1 charset with the import function.

mysql has an option --default-character-set to specify the encoding of the input file. This commit modify the import function so that if encoding exists when using the import function, it will add --default-character-set="value of encoding" to the options of mysql command. This way encoded file in latin1 for example will be imported without error (by default, the file is considered utf8 encoded)

SUMMARY
ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

mysql_db supporting import respecting provided encoding value.

ADDITIONAL INFORMATION

Adding encoding to import function
After defining a latin1 database, there are error when importing sql scripts encoded in latin1 charset with the import function. 

`mysql` has an option `--default-character-set` to specify the encoding of the input file. This commit modify the import function so that if `encoding` exists when using the import function, it will add `--default-character-set="value of encoding"` to the options of mysql command.  This way encoded file in latin1 for example will be imported without error (by default, the file is considered utf8 encoded)
@ansibot

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2019

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

lib/ansible/modules/database/mysql/mysql_db.py:234:161: E501 line too long (165 > 160 characters)

click here for bot help

netmonk added 2 commits Aug 23, 2019
mysql_db.py fix
fixed lint issue, line too long
fixed encoding is not none, neither empty (when there import function is called without a declared encoding value, encoding="" which leads to mysql error for unknown charset)
mysql_db.py fix lint
fixing lint error
@netmonk

This comment has been minimized.

@ansibot ansibot added the stale_ci label Sep 4, 2019

@bmalynovytch

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

Hi @netmonk !
Thank you for your contribution.
I'm gonna give a look at it.
Nobody reviewed it as you forgot to add the label saying you were done with your changes.
You'll find the infos here : https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md#for-pull-request-submitters

@bmalynovytch
Copy link
Contributor

left a comment

You need to add the corresponding documentation, as well as an example.
Good job though 👍

[DOC] mysqldb_py
Updating documentation section to explain this new import option.

@ansibot ansibot removed the stale_ci label Sep 4, 2019

mysqldb_py fixing lint issue
ready_for_review
@bmalynovytch

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

@netmonk when you're done with your changes, just type ready for review (replace spaces by underscores) in the chat to let the bot know you're done.

Update lib/ansible/modules/database/mysql/mysql_db.py
Co-Authored-By: Benjamin MALYNOVYTCH <bmalynovytch@users.noreply.github.com>
@netmonk

This comment has been minimized.

Copy link
Author

commented Sep 4, 2019

ready_for_review

@netmonk

This comment has been minimized.

Copy link
Author

commented Sep 4, 2019

ready_for_review

@bmalynovytch

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

@netmonk you marked my review as fixed but it's not and you don't even express your point of view
I won't review further

@netmonk

This comment has been minimized.

Copy link
Author

commented Sep 4, 2019

@netmonk you marked my review as fixed but it's not and you don't even express your point of view
I won't review further

well, i prefer to have multiple exemples, like there are multiples exemples for others functions of the module.
Often the Ansible documention is really unclear about usable options when using a module. I prefer to have two exemples with two use-cases specific use case.
Is it clearer ?

@bmalynovytch

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

The import function remains the same.
Specifying the encoding isn't a different function, just an argument.

@netmonk

This comment has been minimized.

Copy link
Author

commented Sep 4, 2019

This argument is optionnal, and to be used if needed.
I will follow your suggestion.

@bmalynovytch

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

LGTM
shipit

@netmonk

This comment has been minimized.

Copy link
Author

commented Sep 4, 2019

Do i need to close the pull request now ?

@bmalynovytch

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

No it’ll get closed when merged. Before that other contributors must review and add shipit command to confirm they are ok with your code.

@bmalynovytch

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

bot_status

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

Components

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

Metadata

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

click here for bot help

@Andersson007
Copy link
Contributor

left a comment

@netmonk , thank you for the PR!
Would be cool to see ci test for this case added to the existing tests here test/integration/targets/mysql_db/tasks/.
Could you please make them.
Then we can merge it without any concerns (IMO ci should be added to all prs except the most primitive and doc related).

@netmonk

This comment has been minimized.

Copy link
Author

commented Sep 8, 2019

I will try to do it, need some more time for it. When thinking about it, i just realised that dump is also prone to charset specification. So i can also update the dump function to take encoding into consideration. What do you think about ?

@Andersson007

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2019

I don’t know :) i can’t think anything because i’m not a hardcore MySQL dba, we use it for a couple of non production services for legacy reasons. So I can look at this only in general. Anyway I believe we need tests for every bugfux/new feature PR not to break anything existent by merging them. Would be cool to be sure that all related options and possible cases that might be affected by the code changes are covered.

@netmonk

This comment has been minimized.

Copy link
Author

commented Sep 8, 2019

Im not a dba neither. It's just i just spend the last 6 months advocating against the use of the shell module in Ansible in other dpt of my client company. And then i was asking to automate deployement of a third party software where the mysql scripts are encoded in Latin1 and i had some issue. That is why i decided to look at code and fix it. In order to avoid to use the shell module... :)

@Andersson007

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2019

@netmonk ok, @bmalynovytch approved it, so, if there were ci tests I would approve it too :)

@netmonk

This comment has been minimized.

Copy link
Author

commented Sep 9, 2019

will do. gimme time ! :)
Any pointer on how to run those tests on my laptop ? and more widely how to test ansible code ?

@ansibot ansibot added the stale_ci label Sep 14, 2019

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