-
Notifications
You must be signed in to change notification settings - Fork 319
[DRAFT] Integrated Security Tests #3887
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 draft PR investigates why Integrated Security tests are being skipped in the CI/CD pipeline. The author suspects that the Azure DevOps Library variable $(SupportsIntegratedSecurity) isn't being properly resolved, even though it should always be true. The PR hardcodes this value to true in the pipeline configuration to test whether this resolves the issue.
Changes:
- Hardcoded
SupportsIntegratedSecurity: truein 6 test configurations that use local SQL Server instances (SQL 2019 x64, SQL 2019 x86, SQL 2022 x64, SQL 2022 x86, SQL 2022 named instance, and Always Encrypted tests)
| AzureKeyVaultUrl: $(AzureKeyVaultUrl) | ||
| AzureKeyVaultTenantId: $(AzureKeyVaultTenantId) | ||
| SupportsIntegratedSecurity: $(SupportsIntegratedSecurity) | ||
| SupportsIntegratedSecurity: true |
Copilot
AI
Jan 13, 2026
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 a spelling error in the PR title. "Seciruty" should be "Security".
| AzureKeyVaultUrl: $(AzureKeyVaultUrl) | ||
| AzureKeyVaultTenantId: $(AzureKeyVaultTenantId) | ||
| SupportsIntegratedSecurity: $(SupportsIntegratedSecurity) | ||
| SupportsIntegratedSecurity: true |
Copilot
AI
Jan 13, 2026
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.
Hardcoding configuration values like this removes flexibility and makes it difficult to override this setting for different environments or test scenarios. While this change is appropriate for investigating why tests are being skipped, for a production solution consider:
- Investigating why the Azure DevOps Library variable
$(SupportsIntegratedSecurity)isn't being properly resolved - Ensuring the variable is correctly set in the 'ADO Test Configuration Properties' variable group
- If the variable needs to always be true for these local SQL Server configurations, document this requirement
Since this is marked as a DRAFT PR for experimentation, this approach is acceptable for testing purposes, but shouldn't be merged as-is.
| SupportsIntegratedSecurity: true | |
| SupportsIntegratedSecurity: $(SupportsIntegratedSecurity) |
We seem to be skipping any tests that require Integrated Security. Our PR/CI pipelines appear to intend to use an Azure DevOps Library variable
$(SupportsIntegratedSecurity), which is always true, but the tests still aren't running. I have forced this value to true in the pipelines themselves, and we will see what happens...