testcontainers-dotnet icon indicating copy to clipboard operation
testcontainers-dotnet copied to clipboard

feat: Add a check for MsSql tools path on MsSqlContainer

Open outofrange-consulting opened this issue 1 year ago • 6 comments

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

  • Closes #1220

Follow-ups

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

outofrange-consulting avatar Jul 25 '24 07:07 outofrange-consulting

Deploy Preview for testcontainers-dotnet ready!

Name Link
Latest commit 0fb255daf8e6d9992a10b948c8f79688d3f649a6
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.

netlify[bot] avatar Jul 25 '24 07:07 netlify[bot]

Sure no problem :)

outofrange-consulting avatar Jul 29 '24 07:07 outofrange-consulting

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

kescherCode avatar Aug 05 '24 06:08 kescherCode

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?

mmalvik avatar Aug 14 '24 11:08 mmalvik

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 avatar Aug 14 '24 17:08 HofmeisterAn

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 avatar Aug 26 '24 13:08 HofmeisterAn

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 avatar Aug 29 '24 15:08 HofmeisterAn

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.

HofmeisterAn avatar Aug 29 '24 17:08 HofmeisterAn