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.

Sign up for GitHub

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

Jump to bottom

test: Add ProxySQL to our Docker development stack #6754

Merged
merged 11 commits into from
Mar 29, 2023
Merged

test: Add ProxySQL to our Docker development stack #6754

merged 11 commits into from
Mar 29, 2023

Conversation

beautifulentropy
Copy link
Member

@beautifulentropy beautifulentropy commented Mar 16, 2023 •
edited
Loading

Add an upstream ProxySQL container to our docker-compose. Configure ProxySQL to manage database connections for our unit and integration tests.

Fixes #5873

@beautifulentropy beautifulentropy requested a review from a team as a code owner March 16, 2023 23:56
@beautifulentropy beautifulentropy requested a review from jsha March 16, 2023 23:56
aarongable
aarongable previously approved these changes Mar 24, 2023
View reviewed changes
Copy link
Contributor

@aarongable aarongable left a comment

Choose a reason for hiding this comment

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

(sniping first review because @jsha is focused on rustls stuff this week) LGTM with one nit

test/proxysql/proxysql.cnf Outdated Show resolved Hide resolved
@pgporada pgporada self-requested a review March 27, 2023 16:07
pgporada
pgporada reviewed Mar 27, 2023
View reviewed changes
Copy link
Member

@pgporada pgporada left a comment

Choose a reason for hiding this comment

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

Do we want to connect to proxysql in the integration tests to check arbitrary database output?

test/proxysql/entrypoint.sh Outdated Show resolved Hide resolved
pgporada
pgporada previously approved these changes Mar 27, 2023
View reviewed changes
aarongable
aarongable reviewed Mar 28, 2023
View reviewed changes
Copy link
Contributor

@aarongable aarongable left a comment

Choose a reason for hiding this comment

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

@pgporada said:

Do we want to connect to proxysql in the integration tests to check arbitrary database output?

This suggestion makes sense to me. Phil has now approved without this comment being visibly addressed -- was it discussed offline yesterday? What was the conclusion of that discussion?

@pgporada
Copy link
Member

We didn't discuss it, but changes were pushed based on my other comments. I took the lack of that change as, "No I don't think it's necessary" and approved it anyways.

@beautifulentropy
Copy link
Member Author

We didn't discuss it, but changes were pushed based on my other comments. I took the lack of that change as, "No I don't think it's necessary" and approved it anyways.

Oh, I must have missed this comment. Yeah I can totally make the change here.

aarongable
aarongable approved these changes Mar 29, 2023
View reviewed changes
Copy link
Contributor

@aarongable aarongable left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Maybe update the PR description to have an overview of how proxysql is integrated before merging.

@beautifulentropy beautifulentropy merged commit 511f5b7 into main Mar 29, 2023
@beautifulentropy beautifulentropy deleted the add-proxysql-to-docker branch March 29, 2023 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcpherrinm mcpherrinm mcpherrinm left review comments

@aarongable aarongable aarongable approved these changes

@pgporada pgporada pgporada approved these changes

@jsha jsha Awaiting requested review from jsha jsha is a code owner automatically assigned from letsencrypt/boulder-developers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add ProxySQL to our dev (docker) stack
4 participants
@beautifulentropy @pgporada @mcpherrinm @aarongable