-
Notifications
You must be signed in to change notification settings - Fork 321
Diagnose macOS agent setup problems #3928
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors SQL Server configuration in Azure DevOps pipelines by centralizing password management and adding diagnostic capabilities for macOS agent setup.
Changes:
- Removed password parameter passing through template layers, replacing it with direct use of the
$(Password)variable from the ADO Library "ADO Test Configuration Properties" - Removed temporary password generation/verification steps from the test job template
- Added documentation comments explaining the Password variable source across all SQL Server configuration step templates
- Enhanced macOS SQL Server setup with timestamp-based diagnostic output and improved error message clarity
- Cleaned up trailing whitespace throughout the pipeline files
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| eng/pipelines/common/templates/steps/configure-sql-server-win-step.yml | Removed password parameter, replaced all parameter references with $(Password) variable, added documentation comment, cleaned up trailing whitespace |
| eng/pipelines/common/templates/steps/configure-sql-server-step.yml | Removed password parameter definition and parameter passing to child templates |
| eng/pipelines/common/templates/steps/configure-sql-server-macos-step.yml | Removed password parameter, replaced with $(Password) variable, added documentation, added PS4 timestamp prompt for diagnostics, improved error log message |
| eng/pipelines/common/templates/steps/configure-sql-server-linux-step.yml | Removed password parameter, replaced with $(Password) variable, added documentation comment, cleaned up trailing whitespace |
| eng/pipelines/common/templates/jobs/ci-run-tests-job.yml | Removed password generation and verification steps that created a temporary GUID-based password |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
…to arithmetically add extra time to macOS jobs.
| - name: operatingSystem | ||
| type: string | ||
| default: '' | ||
| values: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ensured we're defining this parameter as an enum everywhere since we're doing string comparisons to make decisions based on it.
| - ${{ if ne(parameters.prebuildSteps, '') }}: | ||
| - ${{ parameters.prebuildSteps }} # extra steps to run before the build like downloading sni and the required configuration | ||
|
|
||
| - powershell: | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We dont' need to generate a GUID and use it as a password. Our Library already has a $(Password) variable meant for SQL Server SA and User passwords.
| echo $SQLCMD_ERRORS | ||
| echo "Errors will be written to: $SQLCMD_ERRORS" | ||
|
|
||
| # Configure the prompt to show the current timestamp so we can see how long each command takes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Diagnostic to see how long each command takes. Azure DevOps refuses to consistently show timestamps.
| displayName: Test job timeout (in minutes) | ||
| type: number | ||
| default: 60 | ||
| default: 70 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no way to supply the job timeout based on arithmetic because of the way ci-run-tests-job is designed, so we need to increase the timeout for all test jobs. Fingers crossed an extra 10 minutes eliminates the macOS cancellations.
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #3928 +/- ##
==========================================
- Coverage 74.93% 67.57% -7.36%
==========================================
Files 269 263 -6
Lines 43247 66191 +22944
==========================================
+ Hits 32407 44731 +12324
- Misses 10840 21460 +10620
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Work in progress.