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

#26 Support init scripts for test containers #35

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

#26 Support init scripts for test containers #35

wants to merge 1 commit into from
+91 −21

Conversation

zzzLobster
Copy link
Contributor

@zzzLobster zzzLobster commented Oct 9, 2023 •
edited

#26 Feature: Support for TC_INITSCRIPT parameter

@zzzLobster zzzLobster requested a review from a team as a code owner October 9, 2023 10:09
zzzLobster
zzzLobster commented Oct 9, 2023
View reviewed changes
src/main/java/org/testcontainers/jooq/codegen/Plugin.java
throw new MojoExecutionException("Error running jOOQ code generation tool", ex);
} finally {
closeClassloader(oldCL, mavenClassloader);
try (var closableContextClassLoader = new ClosableContextClassLoader(getMavenClassloader())) {
Copy link
Contributor Author

@zzzLobster zzzLobster Oct 9, 2023 •
edited

Choose a reason for hiding this comment

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

Reworked it to set context ClassLoader before running the container, to be able loading initScript from classpath.
Unfortunately tests as they are cannot cover this case properly (they successfully work without this change). I can try implementing multi-module test to cover it (to make it failing without the change)

@zzzLobster
Copy link
Contributor Author

zzzLobster commented Dec 1, 2023 •
edited

@sivaprasadreddy , @mzagar, @eddumelendez , could you please merge approved PRs and publish new release?
I do need these changes.
Thank you so much!

@rajadilipkolli
Copy link
Contributor

@kiview can you please approve?

eddumelendez
eddumelendez reviewed Feb 23, 2024
View reviewed changes
Copy link
Member

@eddumelendez eddumelendez left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. I would suggest next time discussing the feature before raising a PR. No immediate action for now, I see some other options

  1. Support copy file
  2. Support volume mount
  3. Execute scripts using ScriptUtils (this PR)

Currently, the suggested approach is using copy files instead due to most of the images provides a fresh initialization folder.

src/main/java/org/testcontainers/jooq/codegen/database/DatabaseProps.java
* Optional
*/
@Parameter
private String initScript;
Copy link
Member

Choose a reason for hiding this comment

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

if scripts support is added then supporting a list would be more flexible.

@zzzLobster
Copy link
Contributor Author

Thanks for the contribution. I would suggest next time discussing the feature before raising a PR. No immediate action for now, I see some other options

1. Support copy file

2. Support volume mount

3. Execute scripts using ScriptUtils (this PR)

Currently, the suggested approach is using copy files instead due to most of the images provides a fresh initialization folder.

@eddumelendez , could you please check my questions in the issue? #26

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eddumelendez eddumelendez eddumelendez left review comments

@sivaprasadreddy sivaprasadreddy sivaprasadreddy approved these changes

@romchellis romchellis romchellis approved these changes

At least 1 approving review is required to merge this pull request.

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

Successfully merging this pull request may close these issues.

None yet
5 participants
@zzzLobster @rajadilipkolli @sivaprasadreddy @eddumelendez @romchellis