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

Fix typo in Dockerfile VOLUME #78

Merged
merged 1 commit into from
Dec 12, 2023
Merged

Fix typo in Dockerfile VOLUME #78

merged 1 commit into from
Dec 12, 2023

Conversation

mnixry
Copy link
Contributor

@mnixry mnixry commented Dec 11, 2023

It acts to create a directory mount at "/[/var/lib/mysql/]/", which is definitely not we want.

It acts to create a directory mount at `"/[/var/lib/mysql/]/"`, which is definitely not we want.
@kou
Copy link
Member

kou commented Dec 11, 2023

Wow, really? How did you check it?

@kou
Copy link
Member

kou commented Dec 11, 2023

Ah, we need a quote to make the path a valid JSON array?

@mnixry
Copy link
Contributor Author

mnixry commented Dec 12, 2023

Wow, really? How did you check it?

You can use docker inspect -f '{{ .Mounts }}' <container name> to check it:

image

The first volume is manually attached (configured in docker-compose), and the second volume is automatically created by the VOLUME statement in the Dockerfile.

If you exec into the container, you'll find a folder named [ in the root directory:

image

Do we need quotes to make the path a valid JSON array?

Not really necessary. Statements like VOLUME /var/lib/mysql or VOLUME ["/var/lib/mysql"] are both acceptable. Since the official Docker MySQL image already declares the same volume location, this line may not be required now.

@kou
Copy link
Member

kou commented Dec 12, 2023

Thanks. I see.
I'll merge this.

Since the official Docker MySQL image already declares the same volume location, this line may not be required now.

In my understanding, we need VOLUME because the official image has it.

@kou kou merged commit 6fc605a into mroonga:master Dec 12, 2023
0 of 2 checks passed
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.

2 participants