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

fix: Get sqlcmd utility file path from container instead of const file path #1221

Merged
merged 7 commits into from
Aug 29, 2024

Conversation

outofrange-consulting
Copy link
Contributor

What does this PR do?

It adds a check on the mssql-tools path which changed during 2022-CU14 update.

Why is it important?

Because from the 2022-CU14 version, Microsoft changed the path to the mssql tools, breaking CI for many folks who have been using the 2022-latest image tag.

Related issues

Follow-ups

I'm not a fan of duplicating the logic between WaitUntil and the MsSqlContainer :'(

Copy link

netlify bot commented Jul 25, 2024

Deploy Preview for testcontainers-dotnet ready!

Name Link
🔨 Latest commit 0fb255d
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-dotnet/deploys/66d07dca0abbd8000860f2cd
😎 Deploy Preview https://deploy-preview-1221--testcontainers-dotnet.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.

@outofrange-consulting outofrange-consulting changed the title fix: check mssql tools path to choose the right one. feat: Add a check for MsSql tools path on MsSqlContainer Jul 25, 2024
Copy link
Collaborator

@HofmeisterAn HofmeisterAn left a comment

Choose a reason for hiding this comment

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

Thank you for preparing the PR. I would like to wait a few more days to see if MS decides whether they will add symlink support (or placing the commands in the base path) or not: microsoft/mssql-docker#892 (comment).

@outofrange-consulting
Copy link
Contributor Author

Sure no problem :)

Geoffrey MARC added 2 commits August 1, 2024 10:21
Expose MsSqlContainer.GetSqlCmdPathAsync as internal
# Conflicts:
#	src/Testcontainers.MsSql/MsSqlBuilder.cs
#	src/Testcontainers.MsSql/MsSqlContainer.cs
@kescherCode
Copy link

Even if Microsoft adds symlink support, changes will be neccessary depending on tool version (especially regarding encryption flags).

@mmalvik
Copy link

mmalvik commented Aug 14, 2024

Thank you for preparing the PR. I would like to wait a few more days to see if MS decides whether they will add symlink support (or placing the commands in the base path) or not: microsoft/mssql-docker#892 (comment).

Is this PR still in consideration for being merged or not?

@HofmeisterAn
Copy link
Collaborator

Yes, but I still hope that Microsoft responds to the issue and decides to address it properly in their image. IIRC, this affects older versions as well. If we need to work around the issue, we will have to fix it for the older versions of MSSQL and different versions of mssql-tools too 🙃.

@HofmeisterAn
Copy link
Collaborator

Since Microsoft is taking their time to address the issue, I think we should move forward with this pull request. Could we adjust the command to determine the sqlcmd file path to something like this?

SQLCMD_PATH=$(find /opt/mssql-tools*/bin/sqlcmd -type f 2>/dev/null | head -n 1)

It should work for all image versions, including new releases. WDYT?

@HofmeisterAn HofmeisterAn added bug Something isn't working module An official Testcontainers module labels Aug 29, 2024
@HofmeisterAn
Copy link
Collaborator

I've changed the strategy and utilized find to find the sqlcmd binary to avoid relying on fixed paths. This should support upcoming versions and changes as well. I also need to apply the changes to ExecScriptAsync(string, CancellationToken) or find a way to share the approach. I just haven't had the time to finalize it yet. We could probably set sqlCmdFilePath only once, similar to what we're doing in Pulsar.

@HofmeisterAn HofmeisterAn changed the title feat: Add a check for MsSql tools path on MsSqlContainer fix: Get sqlcmd utility file path from container instead of const file path Aug 29, 2024
@HofmeisterAn HofmeisterAn merged commit b2699cc into testcontainers:develop Aug 29, 2024
11 checks passed
@HofmeisterAn
Copy link
Collaborator

HofmeisterAn commented Aug 29, 2024

Thanks for the initial PR and your patience 🙏. I think we now have a good approach that should work for new releases (and different MSSQL versions) right out of the box.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working module An official Testcontainers module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: MsSql health check does not complete on newest container image
5 participants