[Azure.AI.AgentServer] Fix initial release issues #53792
Conversation
| <PackageDownload Include="Azure.Sdk.Tools.Testproxy" Version="[$(TestProxyVersion)]" /> | ||
| </ItemGroup> | ||
|
|
||
| <ItemGroup Condition="'$(TargetFramework)' == 'net9.0' |
There was a problem hiding this comment.
This is not going to fly. We generally want consistent dependency versions across all targets. We've not had to do this for any of our ASPNet Core packages yet, why does yours need it?
There was a problem hiding this comment.
our package needs AspNetCore to run server for user, so we need add integration tests, but the packages defined before(Microsoft.AspNetCore.TestHost 8.0.21) doesn't work with net9.0, and net9.0 is required in generated test jobs, so I add this conditional group to bump related packages to 9.*
There was a problem hiding this comment.
That sounds like the best path forward will be to bump the global package, which is going to move to the 10x line very shortly regardless. I think it's worth connecting for a bit and walking through the end-to-end scenario. I'd like to understand where your package is different than the other ASPNET Core target packages that we host in the repository and determine the best long-term approach.
| <RequiredTargetFrameworks>$(LtsTargetFramework)</RequiredTargetFrameworks> | ||
| <TargetFrameworks>$(LtsTargetFramework)</TargetFrameworks> | ||
| <!-- Approved override --> | ||
| <RequiredTargetFrameworks>net9.0;$(LtsTargetFramework);</RequiredTargetFrameworks> |
There was a problem hiding this comment.
Why do you need a specific 9.0 target? Given that we're going to be adding a net10 target in before end-of-month, not sure that we really need this for a preview scenario.
The global 9.0 target exists just because of performance improvements for ModelReaderWriter serialization scenarios.
There was a problem hiding this comment.
actually net9.0 is required by tests, when integration test runs on net9.0 but it not appears in not targets, the test will fail, will start a thread with eng sys team on this
There was a problem hiding this comment.
We execute the tests on .NET 9, but that does not require a net9.0 target. The framework will load net8.0 just fine. I'd like to connect once the Ignite push dies down and dig into your end-to-end scenario more deeply.
| <RequiredTargetFrameworks>$(LtsTargetFramework)</RequiredTargetFrameworks> | ||
| <TargetFrameworks>$(LtsTargetFramework)</TargetFrameworks> | ||
| <!-- Approved override --> | ||
| <RequiredTargetFrameworks>net9.0;$(LtsTargetFramework);</RequiredTargetFrameworks> |
There was a problem hiding this comment.
We don't want to hardcode here. These will need to bump when we move repo targets, so we'll want to figure out a better way to abstract this away if we need to multitarget here. I'm not sure that we do.
Contributing to the Azure SDK
Please see our CONTRIBUTING.md if you are not familiar with contributing to this repository or have questions.
For specific information about pull request etiquette and best practices, see this section.