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

feat(mssql): add WithInitSQL function #2988

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

MattiasMTS
Copy link

@MattiasMTS MattiasMTS commented Feb 17, 2025

What does this PR do?

Add WithInitSQL function to facilitate any initial e.g. DDL scripts to be processed before testing against the container.

Why is it important?

It will improve developer experience and follow the same conventions that exist for other modules, e.g. mysql.

How to test this PR

Run the make test in the folder and it should run. I am using podman, so I modify the containerprovider during that time but should work as is if you're using docker

@MattiasMTS MattiasMTS requested a review from a team as a code owner February 17, 2025 21:38
Copy link

netlify bot commented Feb 17, 2025

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit 6403e18
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/67b6f18d85109e00085190c3
😎 Deploy Preview https://deploy-preview-2988--testcontainers-go.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

I left some comments that need to be addressed before accepting the PR.

It's also important to add the new option to the docs at ./docs/modules/mssql.md, that will land into our docs here: https://golang.testcontainers.org/modules/mssql/

Other than that, the PR looks good, we just need to talk about my comments.

Thanks for your work and time here 🙇

@mdelapenya mdelapenya added the feature New functionality or new behaviors on the existing one label Feb 18, 2025
@MattiasMTS
Copy link
Author

It's also important to add the new option to the docs at ./docs/modules/mssql.md, that will land into our docs here: https://golang.testcontainers.org/modules/mssql/

Sure, I was a bit unsure on adding that myself or if it was generated. As for the structure, ok if I follow the previous conventions or do I need to add e.g. the tag of upcoming release this feature will be included in?

@MattiasMTS MattiasMTS force-pushed the ms/script-helper-fn branch 2 times, most recently from 9b6362b to 1961c15 Compare February 18, 2025 14:24
@MattiasMTS MattiasMTS force-pushed the ms/script-helper-fn branch 2 times, most recently from c932c88 to e3c479e Compare February 18, 2025 14:33
Copy link
Contributor

@stevenh stevenh 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 updates, here's some more feedback based on the latest version

@mdelapenya
Copy link
Member

or do I need to add e.g. the tag of upcoming release this feature will be included in?

Yes, please use the following block:

- Not available until the next release of testcontainers-go <a href="https://github.com/testcontainers/testcontainers-go"><span class="tc-version">:material-tag: main</span></a>

@MattiasMTS MattiasMTS force-pushed the ms/script-helper-fn branch 2 times, most recently from ba02e3f to 8deb865 Compare February 18, 2025 16:09
@MattiasMTS MattiasMTS requested a review from stevenh February 18, 2025 16:11
@MattiasMTS MattiasMTS force-pushed the ms/script-helper-fn branch 2 times, most recently from 3c60861 to d5bd9a1 Compare February 18, 2025 16:39
@MattiasMTS
Copy link
Author

Well, it's as they say
image
thanks for the comments @mdelapenya and @stevenh. Should be ready for somewhat of a final review.

Do you want to extend the scope of WithScripts to support other types of executable, e.g. *.sh etc. I see that is quite typical based on previous modules.

Copy link
Contributor

@stevenh stevenh 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 quick updates, think we're getting close.

Did you see the replies to other threads about using PATH instead of parsing custom image?

@stevenh
Copy link
Contributor

stevenh commented Feb 18, 2025

Well, it's as they say .. thanks for the comments @mdelapenya and @stevenh. Should be ready for somewhat of a final review.

Yer sorry, we've been trying to bring some improvement to the code base, which means that the patterns that you see elsewhere have changed, thanks for your patience and quick changes it really helps!

Do you want to extend the scope of WithScripts to support other types of executable, e.g. *.sh etc. I see that is quite typical based on previous modules.

Good question, as you say some modules are using this for scripts as well as pure data (sql).

I would not have expected that until you mentioned it, I wonder if Scripts is causing the ambiguity, is there a better term? Possibly WithSQL?

@MattiasMTS MattiasMTS force-pushed the ms/script-helper-fn branch 2 times, most recently from d531279 to 547ad7e Compare February 18, 2025 17:58
@MattiasMTS
Copy link
Author

Renamed it to WithInitSQL and also updated the code to use PATH as you suggested, which reduced a bunch of code which is nice.

stevenh
stevenh previously approved these changes Feb 18, 2025
Copy link
Contributor

@stevenh stevenh 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 all your work on this, hope you managed to take away some learnings as I did.

@stevenh
Copy link
Contributor

stevenh commented Feb 18, 2025

@MattiasMTS could you update the PR title and description to match the code please

@MattiasMTS MattiasMTS changed the title feat: add WithScripts and GetSQLCmdPath functions feat(mssql): add WithInitSQL function Feb 18, 2025
@stevenh
Copy link
Contributor

stevenh commented Feb 18, 2025

@mdelapenya my suggestion to include t.Context() fails because its 1.24 we should consider bumping to 1.24 instead of 1.23 to take advantage of that.

In the mean time @MattiasMTS feel free to change that back to context.Background() so we don't delay this PR any further.

@MattiasMTS
Copy link
Author

Done. I also added a test of an old image to verify the PATH of sqlcmd works backwards compatible.

@MattiasMTS MattiasMTS requested a review from stevenh February 18, 2025 20:27
@mdelapenya
Copy link
Member

we should consider bumping to 1.24 instead of 1.23 to take advantage of that.

@stevenh we usually develop the library following Go's release policy: https://golang.testcontainers.org/system_requirements/#go-version, which means we always develop the library in the lower supported version (1.23 at the moment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality or new behaviors on the existing one
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants