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

Support default ports and create file system location #41

Merged
merged 5 commits into from
Nov 8, 2023

Conversation

oruehenbeck
Copy link
Contributor

The following changes are included in this pull request:

  1. If the the port of the database configuration is empty (django doc), remove the port argument from the mysql dump command.
  2. If the location directory path does not exists, it will be created now.
  3. Removed the creation of databases from the export by removing the databases argument from the mysql dump command. This is not necessary because the cursor already uses the correct database and it may cause problems if you import the dump to a database with a different name.

If this pull request is accepted, we would be happy about a new release on pypi.

These changes were suggested by @tobiasfunke1.

add port only if not empty
remove "creating databases" in mysql dump
add mkdir to location used from settings
Copy link

codecov bot commented Nov 7, 2023

Codecov Report

Merging #41 (4a38869) into main (9d78855) will not change coverage.
The diff coverage is 0.00%.

@@          Coverage Diff          @@
##            main     #41   +/-   ##
=====================================
  Coverage   0.00%   0.00%           
=====================================
  Files          8       8           
  Lines        326     326           
=====================================
  Misses       326     326           
Files Coverage Δ
django_migrations_ci/backends/mysql.py 0.00% <0.00%> (ø)
django_migrations_ci/backends/postgresql.py 0.00% <0.00%> (ø)
...ngo_migrations_ci/management/commands/migrateci.py 0.00% <0.00%> (ø)

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@iurisilvio
Copy link
Collaborator

I pushed some changes to your branch:

  • Set default port instead of duplicating the big mysqldump string.
  • Fixed postgres port too.
  • Changed how location is managed, resolving it only for filesystem storages.

Thanks for the contribution! It will be merged and released soon. I just have to fix these tests (not related with your PR).

@iurisilvio iurisilvio changed the title A few small improvements Support default ports and create file system location Nov 8, 2023
@iurisilvio iurisilvio merged commit 081d870 into businho:main Nov 8, 2023
4 of 6 checks passed
@iurisilvio
Copy link
Collaborator

iurisilvio commented Nov 9, 2023

Change released in v0,.8! 🍰

@oruehenbeck
Copy link
Contributor Author

oruehenbeck commented Nov 10, 2023

Hi @iurisilvio,

by inserting a default port instead of omitting the port argument, the following error may occur:

Dump test_abc_local to migrateci-mysql-default-cc09480ef1f0e948e29cb58ae5a6c4bc.
Running shell command mysqldump -h localhost -P 3306 -u abc_local --result-file /tmp/migrateciekflpd5j.sql test_abc_local
Traceback (most recent call last):
    ... stack trace ...
    raise MigrateCIShellException(stderr.decode())
django_migrations_ci.shell.MigrateCIShellException: WARNING: Forcing protocol to  TCP  due to option specification. Please explicitly state intended protocol.

So I think it is better to leave out the port argument at least for the mysqldump command. Do you agree?

@iurisilvio
Copy link
Collaborator

Oh, mysqldump connect on unix socket, I'll fix it! 😞

@iurisilvio
Copy link
Collaborator

Fixed on 0.8.1.

@oruehenbeck oruehenbeck deleted the add-features branch November 16, 2023 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants