Skip to content

Conversation

@rossaddison
Copy link
Contributor

Q A
Is bugfix? ✔️/❌
New feature? ✔️/❌
Breaks BC? ✔️/❌

@rossaddison
Copy link
Contributor Author

Microsoft's Copilot recommended adjusting the mssql.yml workflow specifically with an empty flag for 2017 and also with a 'less specific more general' tools/path. The toolspath recommendation for 2019 and 2022 is more specific however. Hence the possible reason why 2017 was failing in the workflow given the current settings.

@codecov
Copy link

codecov bot commented Jan 28, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.21%. Comparing base (bc770fc) to head (38a9dcb).
⚠️ Report is 11 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master      #53      +/-   ##
============================================
- Coverage     96.33%   96.21%   -0.12%     
+ Complexity      102      100       -2     
============================================
  Files            24       24              
  Lines           300      291       -9     
============================================
- Hits            289      280       -9     
  Misses           11       11              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

rossaddison and others added 2 commits January 28, 2026 12:38
Co-authored-by: Sergei Predvoditelev <sergey.predvoditelev@gmail.com>
Co-authored-by: Sergei Predvoditelev <sergey.predvoditelev@gmail.com>
@vjik
Copy link
Member

vjik commented Jan 28, 2026

@rossaddison see logs: Unable to locate package msodbcsql17. Should we use other package?

First sed: Changes HTTP to HTTPS in the repository URL (more secure and sometimes required)
Second sed: Adds the [signed-by=/usr/share/keyrings/microsoft-prod.gpg] directive to tell apt which keyring to use for verifying packages
This ensures apt can properly verify the Microsoft repository packages using the keyring you stored in the first curl command.
@rossaddison rossaddison requested a review from vjik January 29, 2026 13:59
@rossaddison rossaddison requested a review from vjik January 30, 2026 10:45
@rossaddison
Copy link
Contributor Author

Driver 18 requires encryption setting whereas 17 does not. So 18 with encryption should be used first.

@vjik
Copy link
Member

vjik commented Jan 30, 2026

Driver 18 requires encryption setting whereas 17 does not. So 18 with encryption should be used first.

msodbcsql17 is never used, see: https://github.com/yiisoft/data-cycle/actions/runs/21451684432/job/61781803195?pr=53#step:3:80

@rossaddison
Copy link
Contributor Author

Yes it is not used and is running happily on 18. So I think we just drop legacy support for 17 and use 18. mssql server 2017 should run with odbc-version 18 and then the encryption requirement is consistent across all mssql versions. Will run a different test as close to your original.

curl -fsSL https://packages.microsoft.com/keys/microsoft.asc | gpg --dearmor | sudo tee /usr/share/keyrings/microsoft-prod.gpg > /dev/null
curl -fsSL https://packages.microsoft.com/config/ubuntu/$(lsb_release -rs)/prod.list | sudo tee /etc/apt/sources.list.d/mssql-release.list
sudo apt-get update
sudo ACCEPT_EULA=Y apt-get install -y msodbcsql18 || sudo ACCEPT_EULA=Y apt-get install -y msodbcsql17
Copy link
Member

Choose a reason for hiding this comment

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

Question about msodbcsql17 is actual.

Force the workflow to actually run the msodbcsql17 instead of the msodbcsql18.
The later Ubuntu packages do not have msodbcsql17
@rossaddison
Copy link
Contributor Author

I get this response when the workflow uses ubuntu 20.4:

Evaluating tests.if
Evaluating: success()
Result: true
Requested labels: ubuntu-20.04
Job defined at: yiisoft/data-cycle/.github/workflows/mssql.yml@refs/pull/53/merge
Waiting for a runner to pick up this job...

@rossaddison
Copy link
Contributor Author

Ok yiisoft/data-cycle is currently on ubuntu 20.04 so upgrading to ubuntu 22.04 does the trick. The tests have passed but I will check if the tests actually used msodbcsql17.

@rossaddison
Copy link
Contributor Author

Ok all the latest-2017 php workflow tests actually use msodbcsql17 from ubuntu 22.04 and do not use the encryption requiring msodbcsql18 and the workflow has been improved to include three additional steps namely:

  1. Show Ubuntu Version
  2. Install ODBC Driver
  3. Verify ODBC Driver Installation

So the current workflow has been upgraded to, as you suggested to a newer package, namely ubuntu 22.04 from the older ubuntu 20.04 and three additional steps have been included for verification purposes.

@rossaddison rossaddison closed this Feb 1, 2026
@rossaddison rossaddison reopened this Feb 1, 2026
@rossaddison rossaddison requested a review from vjik February 1, 2026 11:53
@rossaddison rossaddison requested a review from vjik February 1, 2026 18:24
Copy link
Member

@vjik vjik left a comment

Choose a reason for hiding this comment

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

Why do you revert changes from previous reviews?

@rossaddison rossaddison requested a review from vjik February 1, 2026 19:06
@vjik
Copy link
Member

vjik commented Feb 1, 2026

@rossaddison Do you use LLM?

@rossaddison
Copy link
Contributor Author

rossaddison commented Feb 1, 2026

I have been using LLM. Yes not always that consistent. I only use a small subscription of LLM. Most of last month coding without it. The md and mssql.yml was produced with copilot. Useful to see alternatives and learning.

uses: actions/checkout@v3

uses: actions/checkout@v6
Copy link
Member

Choose a reason for hiding this comment

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

Remove unexpected spaces

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants