feat(postgres): add WithOrderedInitScripts for Postgres testcontainers#3093
feat(postgres): add WithOrderedInitScripts for Postgres testcontainers#3093AlisCode wants to merge 3 commits intotestcontainers:mainfrom
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
A first look and this looks good. Could you please add tests verifying the new behavior? |
|
I sure can, would you mind pointing me to a test that I can take inspiration from ? |
|
Sure, please take as look here: |
|
Hi! I've added a test that checks that both scripts are executed in the order they are provided. If the behavior changes or becomes inconsistent somehow, and insert-user.sql gets executed first, then the test should break. |
0ab0417 to
6fca103
Compare
mdelapenya
left a comment
There was a problem hiding this comment.
LGTM, just added some minor comments
Thanks!
modules/postgres/postgres.go
Outdated
| return func(req *testcontainers.GenericContainerRequest) error { | ||
| for idx, script := range scripts { | ||
| filename := filepath.Base(script) | ||
| containerFilePath := fmt.Sprintf("%06d-%s", idx, filename) |
There was a problem hiding this comment.
nit, not a blocker: let's use a smaller number. Not sure if we need thousands of files 😅. Please update the tests too if agreed
| containerFilePath := fmt.Sprintf("%06d-%s", idx, filename) | |
| containerFilePath := fmt.Sprintf("%03d-%s", idx, filename) |
There was a problem hiding this comment.
I guess we need to update the tests here 8c32f0f#diff-929bacfd366ab1235318667cb165ce78b7ed08b9c56a0d48f5a29ef6b4379b00R324
There was a problem hiding this comment.
Oops. Sorry for the noise :)
|
@mdelapenya I've taken the time to address all those comments :) Enjoy! |
modules/postgres/postgres.go
Outdated
| } | ||
|
|
||
| // Adds an initialization script to the Postgres container | ||
| func appendInitScript(req *testcontainers.GenericContainerRequest, hostFilePath string, containerFilePath string) { |
There was a problem hiding this comment.
BTW, we just merged an option to append container files, I think we can simplify this code here, just calling the WithFiles option: https://golang.testcontainers.org/modules/postgres/#withfiles
Thoughts?
| } | ||
|
|
||
| // Adds the file located at hostFilePath as an init script that postgres will launch | ||
| func buildInitScriptContainerFile(hostFilePath string, containerFilePath string) testcontainers.ContainerFile { |
There was a problem hiding this comment.
question: do we need this simple function that just returns the struct? I'd consider removing it as it does not do anything special inside. But I'll let you decide
There was a problem hiding this comment.
You asked to share some code between WithOrderedInitScripts and WithInitScripts, so this function does just that. The only alternative I can think of is duplicating the ContainerFile declaration in the 2 functions, so I believe this is better?
There was a problem hiding this comment.
Yes, I asked that before having the new WithFiles options, which now removes the duplication. I'm fine having it as a function, although with the current implementation I see it as a pass-through function. In any case, it's not a blocker to me.
Once the docs are added, I'm happy to merge this 😊
mdelapenya
left a comment
There was a problem hiding this comment.
LGTM; although I'd like to have the new option documented at docs/modules/postgres.md. Please do not forget adding the Not available... marker 🙏 (https://github.com/testcontainers/testcontainers-go/blob/main/docs/modules/arangodb.md?plain=1#L62)
|
@AlisCode friendly ping 🙏 |
|
@AlisCode given this PR is sent from your main branch, I cannot contribute to it with the requested changes. I'm pushing a new branch with all your commits, adding mine on top, so that you appear as contributor. |
|
Closing as there is a superseding PR. When a release is done, I'll update the release notes to include your name as the original author of that PR Thanks for contributing to the project! 🙇 |
Type of change: Enhancement
What does this PR do?
As suggested in the discussion, this adds a brand new option to add init scripts to your Postgres testcontainer, and run them in the order that the user provides them to the function.
It also further documents
WithInitScriptsto clarify in what order the scripts are executed.Why is it important?
The files that are passed to
WithInitScriptsare copied inside the container, then run in alphanumerical order. I believe this is surprising behavior, as I expect the init scripts to be run in the order I passed them to testcontainers.Someone ran into this behavior before and raised an issue : #2542
Related issues
How to test this PR
Try using the newly-added function
WithOrderedInitScriptsfor the postgres container.Any setup using
WithInitScriptsshould work just like before.