Skip to content

Conversation

@David-Engel
Copy link
Contributor

@David-Engel David-Engel commented Aug 2, 2024

This PR switches macOS jobs to install Docker and a local SQL Server container instance instead of connecting to an Azure SQL DB instance for testing. This will allow us to get rid of the network permissions required for the mac agents to access Azure SQL DB and we can also get rid of the AADServicePrincipal connection info.

The ConfigurableIpPreferenceTest had to be adjusted to not run on macOS when targeting localhost as I couldn't get IPv6 working. It appears IPv6 in Docker is only supported on Linux:

https://docs.docker.com/engine/daemon/ipv6/

IPv6 is only supported on Docker daemons running on Linux hosts.

It might be coming soon for macOS, though: docker/for-mac#1432

Additions: (these should make #3094 obsolete)

Update: This also fixes CI for PRs from forks.
PRs from forks won't have access to any secrets (tightened security policy) so they won't be able to run the AE enclave jobs and a few tests that exercised AAD Service Principal Secret methods. Any PRs that break those from forks may only be caught by the nightly scheduled CI runs against main.

You can see fewer jobs ran in this screenshot for this PR compared to #3108:
image

  • Change tests against local SQL Server to always generate a random password for local accounts.
  • Change jobs against local SQL Server to not include AAD connection tests.
  • Update some configs to not pass secret variables to jobs if they aren't available to the pipeline. See Testing CI #3108 for a comparison of CI pipeline output from a branch in this repo versus a fork (this PR).

Code coverage difference shows 66% from this fork PR and 72% from #3108.

@codecov
Copy link

codecov bot commented Dec 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.81%. Comparing base (2598b3c) to head (f99fbcd).
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2750      +/-   ##
==========================================
+ Coverage   72.71%   72.81%   +0.10%     
==========================================
  Files         283      283              
  Lines       58996    58997       +1     
==========================================
+ Hits        42896    42959      +63     
+ Misses      16100    16038      -62     
Flag Coverage Δ
addons 92.58% <ø> (ø)
netcore 75.64% <ø> (+0.16%) ⬆️
netfx 71.23% <ø> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@David-Engel David-Engel marked this pull request as ready for review December 13, 2024 17:51
@mdaigle
Copy link
Contributor

mdaigle commented Jan 2, 2025

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Don't test Azure AAD connectivity in localhost jobs
Don't pass secrets if variables are empty
@dotnet dotnet deleted a comment from azure-pipelines bot Jan 13, 2025
@dotnet dotnet deleted a comment from azure-pipelines bot Jan 13, 2025
@dotnet dotnet deleted a comment from azure-pipelines bot Jan 13, 2025
@dotnet dotnet deleted a comment from azure-pipelines bot Jan 13, 2025
@dotnet dotnet deleted a comment from azure-pipelines bot Jan 13, 2025
@dotnet dotnet deleted a comment from azure-pipelines bot Jan 13, 2025
@dotnet dotnet deleted a comment from azure-pipelines bot Jan 13, 2025
@dotnet dotnet deleted a comment from azure-pipelines bot Jan 13, 2025
@dotnet dotnet deleted a comment from azure-pipelines bot Jan 13, 2025
@dotnet dotnet deleted a comment from azure-pipelines bot Jan 13, 2025
@dotnet dotnet deleted a comment from azure-pipelines bot Jan 13, 2025
@David-Engel
Copy link
Contributor Author

/azp run CI-SqlClient

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@David-Engel
Copy link
Contributor Author

/azp run CI-SqlClient

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@David-Engel
Copy link
Contributor Author

/azp run CI-SqlClient

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@David-Engel
Copy link
Contributor Author

/azp run CI-SqlClient

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@David-Engel
Copy link
Contributor Author

/azp run CI-SqlClient

@azure-pipelines
Copy link

Azure Pipelines failed to run 1 pipeline(s).

@David-Engel
Copy link
Contributor Author

/azp run CI-SqlClient

@azure-pipelines
Copy link

Azure Pipelines failed to run 1 pipeline(s).

@David-Engel
Copy link
Contributor Author

/azp run CI-SqlClient

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@David-Engel
Copy link
Contributor Author

/azp run CI-SqlClient

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@David-Engel David-Engel requested a review from a team January 16, 2025 22:29
Copy link
Contributor

@benrr101 benrr101 left a comment

Choose a reason for hiding this comment

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

This all looks pretty decent to me, especially if it unblocks fork builds.
Only thing I'm not sure about - this removes tests for macos against azure sql?

@mdaigle
Copy link
Contributor

mdaigle commented Jan 23, 2025

This all looks pretty decent to me, especially if it unblocks fork builds. Only thing I'm not sure about - this removes tests for macos against azure sql?

@David-Engel thoughts on adding back a mac-azure run only for non-forked builds? You mentioned above that we can remove some network permissions. Is that still a benefit given the forked build fix included here.

@David-Engel
Copy link
Contributor Author

thoughts on adding back a mac-azure run only for non-forked builds? You mentioned above that we can remove some network permissions. Is that still a benefit given the forked build fix included here.

@mdaigle The original reason to eliminate macOS+Azure was to get rid of usage of the AAD SP Secret (the "eliminate secrets" compliance task). We can't do AD Managed Identity from the Azure-hosted mac pools, so we don't have a password-less auth option for mac. Moving Mac to test against localhost honestly may get us more code coverage than testing Mac against Azure SQL DB. There are a lot of tests that are skipped against Azure SQL. If we want to do Mac + Azure in the future (honestly not a very high priority, in my mind since Linux + Azure is so close to Mac + Azure), we may be able to leverage a new Mac hosting service in MS that we are looking at for ODBC, which might eventually support AAD Managed Identity.

Co-authored-by: samsharma2700 <[email protected]>
@David-Engel David-Engel merged commit f0fef6a into dotnet:main Jan 23, 2025
5 of 123 checks passed
@David-Engel David-Engel deleted the macosdocker branch January 23, 2025 22:11
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.

5 participants