Skip to content
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

Search MSI migration + .NET 9 TFM support #10248

Merged
merged 2 commits into from
Nov 7, 2024
Merged

Conversation

advay26
Copy link
Member

@advay26 advay26 commented Nov 5, 2024

Addresses https://github.com/NuGet/Engineering/issues/5439
Part of https://github.com/NuGet/Engineering/issues/5512

Config changes PR: https://nuget.visualstudio.com/NuGetMicrosoft/_git/NuGetDeployment/pullrequest/2541

This adds MSI support for the search jobs and the search service.

We also add .NET TFMs to our list of "Supported Frameworks" so that they show up as computed frameworks on the frameworks tab and in the search index. The other changes in the SupportedFrameworks.cs file were just reordering to make it a bit more readable.

I also had to make some tests a bit less sensitive to changes. Earlier we were trying to match an exact list of 'computed' compatible frameworks in some tests. With that setup, we would need to keep updating the test cases every time a new .NET TFM is added. I've made the tests less sensitive now, so they still check for the same behavior, but don't need the test case list to be exhaustive.

@advay26 advay26 requested a review from a team as a code owner November 5, 2024 20:50
options.Value.StorageConnectionString = options.Value.StorageConnectionString.Replace("SharedAccessSignature=?", "SharedAccessSignature=");

return new BlobServiceClient(options.Value.StorageConnectionString);
var storageMsiConfiguration = c.Resolve<IOptionsSnapshot<StorageMsiConfiguration>>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need the registration of the BlobServiceClient?
Most of the references in the common stuff should only request BlobServiceClientFactory now, but this registration is probably still necessary if we request the type specifically in any of the search stuff. Otherwise, we can probably clean this up.
Either way, not a huge deal.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure either. This might not be needed any more. I'll try to test it without this and remove it if I can.

I do need to get this work done somewhat soon-ish, so if I feel like I need to move quicker, I'll file this as a follow-up issue and test+remove it later.

@@ -224,6 +224,39 @@ public static BlobServiceClient CreateBlobServiceClient(
}
}

public static BlobServiceClientFactory CreateBlobServiceClientFactory(
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 I like the idea of handling this at this level.

@advay26 advay26 merged commit 9d10a11 into dev Nov 7, 2024
3 checks passed
@advay26 advay26 deleted the advay26-search-migration branch November 7, 2024 20:47
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