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

Error: FAILED: Directory already exists #571

Closed
aviskarkc10 opened this issue Jun 3, 2019 · 7 comments

Comments

@aviskarkc10
Copy link
Contributor

commented Jun 3, 2019

I am trying to initialize my migration inside a docker image with the command:
alembic init {dir_name}

For this I will have to map the volume in my system as follows:

docker run -it -v $(pwd)/migrations:/app/migrations {docker_image_name} alembic init migrations

But this throws the error FAILED: Directory migrations already exists because we are checking the following condition:

if os.access(directory, os.F_OK):
        raise util.CommandError("Directory %s already exists" % directory)

Because of this, I won't be able to initialize my migrations as without volume mapping there is no way to persist the migrations folder in my system.

Is there any reason why we are doing so?
Maybe we should change this implementation to ask the user if they want to overwrite their current migrations?

@zzzeek zzzeek added the question label Jun 3, 2019

@zzzeek

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

Because of this, I won't be able to initialize my migrations as without volume mapping there is no way to persist the migrations folder in my system.

it is standard practice that when a unix application is to create a directory and put things into it, that it would not proceed if the directory is already present, because this indicates existing intent. Normally then, I would suggest you create your mount point one directory upwards of where this subdirectory is to be.

However, as I sought to illustrate an example of how this practice is common, it turns out that git, the best example, has since refined their behavior to check if the directory exists and is also non-empty:

[classic@photon2 tmp]$ mkdir alembic
[classic@photon2 tmp]$ git clone git@github.com:sqlalchemy/alembic.git
Cloning into 'alembic'...
remote: Enumerating objects: 73, done.
remote: Counting objects: 100% (73/73), done.
remote: Compressing objects: 100% (68/68), done.
remote: Total 9519 (delta 28), reused 47 (delta 2), pack-reused 9446
Receiving objects: 100% (9519/9519), 1.91 MiB | 1.42 MiB/s, done.
Resolving deltas: 100% (7067/7067), done.
[classic@photon2 tmp]$ git clone git@github.com:sqlalchemy/alembic.git
fatal: destination path 'alembic' already exists and is not an empty directory.

so this is an enhancement to the existing feature, and looks like this:

diff --git a/alembic/command.py b/alembic/command.py
index f8c81cc..54d7ad1 100644
--- a/alembic/command.py
+++ b/alembic/command.py
@@ -36,19 +36,21 @@ def init(config, directory, template="generic"):
      use.
 
     """
-
-    if os.access(directory, os.F_OK):
-        raise util.CommandError("Directory %s already exists" % directory)
+    if os.access(directory, os.F_OK) and os.listdir(directory):
+        raise util.CommandError(
+            "Directory %s already exists and is not empty" % directory
+        )
 
     template_dir = os.path.join(config.get_template_directory(), template)
     if not os.access(template_dir, os.F_OK):
         raise util.CommandError("No such template %r" % template)
 
-    util.status(
-        "Creating directory %s" % os.path.abspath(directory),
-        os.makedirs,
-        directory,
-    )
+    if not os.access(directory, os.F_OK):
+        util.status(
+            "Creating directory %s" % os.path.abspath(directory),
+            os.makedirs,
+            directory,
+        )
 
     versions = os.path.join(directory, "versions")
     util.status(



it's still not fully correct since "foo" might exist and not be a directory, but that at least raises "not a directory". Also, I never wrote any tests for this and I'd like to do that too.

@zzzeek zzzeek added this to the fasttrack milestone Jun 3, 2019

@zzzeek zzzeek removed the question label Jun 3, 2019

@aviskarkc10

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

Thanks for getting back. I think checking if the file is non-empty should work perfectly. Let me know if I can help. I'd be happy to send a PR.

@zzzeek

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

sure though it would be nice if there were tests for this in tests/test_command.py but it would be a litle involved.

@aviskarkc10

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

Sure. I will give it a shot.

@sqla-tester

This comment has been minimized.

Copy link
Collaborator

commented Jul 17, 2019

Aviskar KC has proposed a fix for this issue in the master branch:

Allow init into existing but empty directory https://gerrit.sqlalchemy.org/1303

@zzzeek

This comment has been minimized.

Copy link
Member

commented Jul 18, 2019

you're in. thanks!

@aviskarkc10

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2019

Thanks @zzzeek. You were really patient with me. Appreciate it.

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