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

improve: expose mysql-server on port 3307 and improve environment variable naming #1359

Merged
merged 9 commits into from
Apr 5, 2022

Conversation

ikesau
Copy link
Member

@ikesau ikesau commented Apr 4, 2022

A bit of scope creep on this ticket, but hopefully worth it.

Changes

  1. The mysql-server container now exposes the its server on port 3307 instead of 3306. This will allow people to run local MySQL instances and prevent errors occurring due to port conflicts

  2. Standardizes environment variable naming across our Makefile, docker-compose, and server code.

Now everything is either:

  • GRAPHER_DB_X
  • WORDPRESS_DB_X
  • DB_ROOT_X - a slightly misleading prefix: refers to both the literal root user password as well as the mysql-server host for other containers to access with.

Questions

  • How will we merge and migrate this safely?

export const GRAPHER_DB_HOST: string =
serverSettings.GRAPHER_DB_HOST ?? "localhost"
export const GRAPHER_DB_PORT: number =
parseIntOrUndefined(serverSettings.DB_PORT) ?? 3307
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe for the default here we should try to stay with 3306? For the blessed setting we override this anyhow but I'm not sure if some dev using local mysql somewhere might not have set this and then it's nicer to stay with the upstream default

Copy link
Contributor

@danyx23 danyx23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and much nicer to have this on a non-default port! Two minor improvement requests for making this less error-prone for people with an existing .env file (I also mentioned these in slack):

  1. .env file validation doesn't kill the make run if a variable is not present
  2. I think we should add a warning if port is set to 3306. We now have a bunch of GSoC people who use this setup that we can't easily communicate with and when we merge this they will have to adjust their .env file or be faced with somewhat obscure database issues

@ikesau
Copy link
Member Author

ikesau commented Apr 5, 2022

@danyx23 done!

Very strange how valdiate.env was behaving. I've tweaked it to consistently exit, though it will break out of the script at the very first unset variable, as opposed to listing them all and then exiting 😢

I also realized I wasn't actually doing anything with the X_DB_PORT variables, because it was hard-coded in the docker-compose files. I've now made that dynamic too (it uses the Grapher port, as we currently don't support having two separate MySQL servers)

And finally, added a warning if your port is still set to 3306 👍

@danyx23
Copy link
Contributor

danyx23 commented Apr 5, 2022

Nice! Testing the port doesn't work for me yet. If I have the port at 3306 then I get this error message in make and the script pauses until I ack a message in VS code about the process having terminated (weird interaction between make and the VS Terminal there)

/bin/sh: 1: [: 3306: unexpected operator

I use zsh, maybe that makes it use /bin/sh instead of bash or something?

@ikesau ikesau merged commit fcbba84 into master Apr 5, 2022
@ikesau ikesau deleted the default-3307 branch April 5, 2022 19:22
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