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

canned postgresql #98

Closed

Conversation

borgoat
Copy link

@borgoat borgoat commented Sep 28, 2019

Ciao @gianarb @alde @mraerino

I took inspiration from #59 also to address #19
I tried to implement some of the suggestions by @mraerino in #59 ..

Let me know if I should change anything else!

return nil, err
}

for {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this is because the postgres container "bounces" and logs the "database system is ready to accept connections" twice?

In the java testcontainer that's handled by having the Wait code have a state and wait for it twice.

I think I prefer this, as it keeps the Waits simple

Copy link
Author

@borgoat borgoat Sep 29, 2019

Choose a reason for hiding this comment

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

Yes, I don't understand why the Postgres container is doing that, but basically the process gets restarted, so you'd have to wait for the second "database system is ready to accept connections". So I tried to mimic the Java implementation for the generic JDBC containers.

I realised though, the wait strategy I put there is basically pointless? 🤔

I have another proposal to make the same wait process into a proper SQLStrategy wait strategy.

wait/sql.go Outdated
for {
_, err := db.ExecContext(ctx, "SELECT 1")
if err != nil {
time.Sleep(500 * time.Millisecond)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe there should be a max wait time, just in case?

Copy link
Author

Choose a reason for hiding this comment

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

Whoops... you're right. I completely forgot.
I did the same as in wait/log.go now. Also added the fluent builders as in LogStrategy for consistency

gianarb pushed a commit that referenced this pull request Oct 2, 2019
This is related to the work @giorgioazzinnaro is doing here
#98 .

Now you can declare how many times a log line should appear before
having the container declared as ready.

Signed-off-by: Gianluca Arbezzano <gianarb92@gmail.com>
gianarb pushed a commit that referenced this pull request Oct 3, 2019
This is related to the work @giorgioazzinnaro is doing here
#98 .

Now you can declare how many times a log line should appear before
having the container declared as ready.

Signed-off-by: Gianluca Arbezzano <gianarb92@gmail.com>
gianarb pushed a commit that referenced this pull request Oct 3, 2019
This is related to the work @giorgioazzinnaro is doing here
#98 .

Now you can declare how many times a log line should appear before
having the container declared as ready.

Signed-off-by: Gianluca Arbezzano <gianarb92@gmail.com>
gianarb pushed a commit that referenced this pull request Oct 3, 2019
This is related to the work @giorgioazzinnaro is doing here
#98 .

Now you can declare how many times a log line should appear before
having the container declared as ready.

Signed-off-by: Gianluca Arbezzano <gianarb92@gmail.com>
@gianarb
Copy link
Member

gianarb commented Oct 3, 2019

Sweet! Do you mind to add a godoc example as well?! Thanks a lot

@borgoat
Copy link
Author

borgoat commented Oct 4, 2019

Yeah, I think that'd be useful! Will do over the weekend.

As for the wait strategy, do you guys think we should keep the SQL client or go back to LogStrategy with multiple occurrences?

@borgoat borgoat mentioned this pull request Oct 15, 2019
@gordonbondon
Copy link

@giorgioazzinnaro I've created a PR to your branch to add support for custom dockerfiles borgoat#1

@borgoat
Copy link
Author

borgoat commented Oct 25, 2019

I merged #105 #98 and #102 in the master of my fork to see how the godoc turns out, so here it is:
https://godoc.org/github.com/giorgioazzinnaro/testcontainers-go/canned

Any other suggestions on how to proceed with this?

This was referenced Oct 25, 2019
@nikolayk812
Copy link

It would be great to have this feature being merged and released

nikolayk812 pushed a commit to nikolayk812/testcontainers-go that referenced this pull request Nov 21, 2019
nikolayk812 pushed a commit to nikolayk812/testcontainers-go that referenced this pull request Nov 21, 2019
nikolayk812 pushed a commit to nikolayk812/testcontainers-go that referenced this pull request Nov 21, 2019
nikolayk812 pushed a commit to nikolayk812/testcontainers-go that referenced this pull request Nov 28, 2019
@DarrienG
Copy link

This looks pretty good. What's blocking it from being merged? Glad to help if I can.

@phss
Copy link

phss commented May 29, 2020

Hey folks, just wanted to check again how's the progress with this PR? If there's anything I can do to help, let me know!

@codyaray
Copy link

codyaray commented Nov 5, 2020

Looks like a file named wait/sql.go has been merged to upstream, but doesn't match the expectations here.

./postgres_container.go:115:30: not enough arguments in call to wait.ForSQL
	have (func(context.Context, wait.StrategyTarget, <T>) (driver.Connector, error), map[string]interface {})
	want (nat.Port, string, func(nat.Port) string)
./postgres_container.go:136:93: undefined: wait.SQLVariables

@borgoat
Copy link
Author

borgoat commented Nov 7, 2020

I guess we need to include the maintainers again in the discussion, I think we were still stuck at understanding whether it's sensible to throw all the possible canned containers in the root module, or rather create a go module for each of them (so that pulling the main module won't bring along a dependency on Redis, Postgres, MySQL, etc ... clients). Since we last discussed this the support for Go modules has greatly improved

@gianarb @mdelapenya what's your opinion on this?

@mdelapenya
Copy link
Member

We started this PR, which could be the seed for modules: #618

It creates examples to be consumed by the website, but turning them into modules would be a matter of adjusting the templates.

@mdelapenya
Copy link
Member

Given #618 has been merged, we created #636, where you can find how to contribute new modules :)

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.

9 participants