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 for sql wait strategy #214

Closed
wants to merge 10 commits into from
Closed

test for sql wait strategy #214

Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7623f53
test for sql wait strategy
Jun 16, 2020
5090a81
Merge branch 'main' into chore/write-test-for-sql-wait
mdelapenya Sep 29, 2022
5d2ee47
fix: tabs
mdelapenya Sep 29, 2022
450b078
fix: tabs
mdelapenya Sep 29, 2022
91896d4
fix: wrong resolve conflicts
mdelapenya Sep 29, 2022
18f86e4
fix: wrong resolve conflicts
mdelapenya Sep 29, 2022
f1dee39
Merge branch 'main' into chore/write-test-for-sql-wait
mdelapenya Nov 28, 2022
5703932
fix: format
mdelapenya Nov 28, 2022
bb8ec0e
chore: remove extra settings for db tuning
mdelapenya Nov 28, 2022
4c77f11
Merge branch 'main' into chore/write-test-for-sql-wait
mdelapenya Mar 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions wait/sql.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,9 @@ func ForSQL(port nat.Port, driver string, url func(string, nat.Port) string) *wa
Port: port,
URL: url,
Driver: driver,
startupTimeout: defaultStartupTimeout(),
PollInterval: defaultPollInterval(),
// Not using the default duration here because it is too low. It will never work
startupTimeout: 20 * time.Second,
PollInterval: time.Second * 1,
query: defaultForSqlQuery,
}
}
Expand Down Expand Up @@ -88,6 +89,11 @@ func (w *waitForSql) WaitUntilReady(ctx context.Context, target StrategyTarget)
return fmt.Errorf("sql.Open: %v", err)
}
defer db.Close()

db.SetConnMaxLifetime(0)
db.SetMaxIdleConns(3)
db.SetMaxOpenConns(3)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Although the code LGTM, I'm a bit reluctant to hardcode this value here without giving the user the ability to set them in the wait strategy, according to their own requirements for the database under test. If we add them as part of the SQL strategy, then I think it's fine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thinking about this twice, these settings should be added in a separate PR. Will send a commit on top removing them, and opening a discussion to make this configurable


for {
select {
case <-ctx.Done():
Expand Down