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

patch: use docker compose v2 and update MYSQL_DATABASE env var #2870

Merged
merged 2 commits into from
Oct 17, 2023

Conversation

juanjuanzero
Copy link
Contributor

small changes to development.md file and makefile

  • updates to use docker compose v2
  • fixes environment variable for MYSQL_DATABASE was causing failing tests when running make test-examples

I'm new to this repo and to contributing, please let me know if i had missed anything.

@andrewmbenton
Copy link
Collaborator

Thanks for the contribution! The docker compose change is good.

The docs regarding MySQL connection string variables represent the defaults as encoded in https://github.com/sqlc-dev/sqlc/blob/main/internal/sqltest/mysql.go so we shouldn't change those (although the formatting of that "table" could be improved). I think it would be appropriate to add a note at the bottom of that section explaining that you will need to create the dinotest database or set the environment variable to a non-default value in order for the tests to run.

@juanjuanzero
Copy link
Contributor Author

Thanks for the contribution! The docker compose change is good.

The docs regarding MySQL connection string variables represent the defaults as encoded in https://github.com/sqlc-dev/sqlc/blob/main/internal/sqltest/mysql.go so we shouldn't change those (although the formatting of that "table" could be improved). I think it would be appropriate to add a note at the bottom of that section explaining that you will need to create the dinotest database or set the environment variable to a non-default value in order for the tests to run.

Thanks for the warm welcome and the feedback. I see, we do use dinotest as a default. I'll update the docker-compose file instead to set the MYSQL_DATABASE as dinotest instead of mysql unless that is intended for something else.

@kyleconroy kyleconroy merged commit eac251f into sqlc-dev:main Oct 17, 2023
8 checks passed
@andrewmbenton
Copy link
Collaborator

I'll update the docker-compose file instead to set the MYSQL_DATABASE as dinotest instead of mysql unless that is intended for something else.

Hmm this brings up a good point. I'm not sure why we don't have the containers start up with the dinotest database created by default. Looks like @kyleconroy just merged this so I guess he thinks it makes sense :)

Although then I think we should do it for the postgres containers too. And while we're at it, we could change the name of the default db from dinotest to sqlctest (dinosql was the original name of sqlc).

This pull request was closed.
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.

3 participants