Conversation
dev.yml
Outdated
| name: Waiting for DBs to be operational | ||
| met?: ./scripts/helpers/wait-for-dbs.sh | ||
| meet: ":" | ||
| meet: docker-compose up -d |
There was a problem hiding this comment.
The above two steps felt like they could be merged into one. I mean in the end after setting up docker-compose we want to see if the local test containers are up or not.
There was a problem hiding this comment.
Won't this make it run a lot slower? It will try to met? which will take a whole long time to timeout (If there is a timeout), then it will create the docker-compose. I set it up in this order with a "no-op" to ensure that the containers are at least warming up when the script starts
There was a problem hiding this comment.
Did you try to run this with the docker-compose being down? Or did you run it when it was already up?
Manan007224
force-pushed
the
fix-documentation
branch
from
ba0e3de to
22a6969
Compare
| logs: | ||
| desc: "See the DB logs (ctrl-c + ctrl-c to exit)" | ||
| run: docker-compose logs -f | ||
| mysql: |
There was a problem hiding this comment.
That's a really good addition 👍
EtienneBerubeShopify
left a comment
EtienneBerubeShopify
left a comment
There was a problem hiding this comment.
Few things to clarify, but overall looks good 👍
spec/integration/database.yml
Outdated
| proxysql: | ||
| host: proxysql | ||
| user: root | ||
| user: remote-admin |
There was a problem hiding this comment.
I don't think this should be changed. This file is used to connect to the underlying database and not the admin. I added these changes when changing the test environment, but the changes are only used in #112
scripts/proxysql/proxysql.cnf
Outdated
| threads=8 | ||
| max_connections=100000 | ||
| interfaces="0.0.0.0:3306" | ||
| interfaces="0.0.0.0:6033" |
There was a problem hiding this comment.
Wouldn't we want to be able to communicate with MySQL on port 3306? So that everything is consistent?
EtienneBerubeShopify
left a comment
EtienneBerubeShopify
left a comment
There was a problem hiding this comment.
nit: little question about port selection, but other than that LGTM 👍
Manan007224
force-pushed
the
fix-documentation
branch
from
77e5b88 to
9d6154c
Compare
changes changes changes changes changes fix tests changes
Manan007224
force-pushed
the
fix-documentation
branch
from
9793ffc to
2691562
Compare
2 participants
I am fixing two things in this PR
README.mdfor specs - This file had references to some redundant scripts which we don't exists anymore and also added references in respect to our new docker-compose based setup.proxysql setup via docker-compose.