Fix WithReference to only use IResourceWithEndpoints#14254
Fix WithReference to only use IResourceWithEndpoints#14254Falco20019 wants to merge 10 commits intomicrosoft:mainfrom
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 14254Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 14254" |
|
Breaking change no? |
|
As the other interface inherits it, I wouldn't think so. I can't see the unit tests here in GitHub Actions right now and won't be near a PC before Monday. If they still work, it shouldn't be breaking. |
Ok, now I understand what you meant. It's not clashing with |
…urceWithServiceDiscovery
|
I sadly don't get it to compile locally on macOS with Docker Desktop following https://github.com/dotnet/aspire/blob/main/docs/machine-requirements.md Neither through DevContainers nor natively as I think offering https://dev.azure.com/dnceng-public/public/_build/results?buildId=1276039&view=results seems to hint that no tests have been executed by the CI, is that normal? |
|
@davidfowl Should be non-breaking now. |
Should they be made obsolete with a note that they'll be removed in the future? |
|
@JamesNK I think this would then show obsolete messages all over the place in all cases where the more specific interface is used, right? |
There was a problem hiding this comment.
Thanks a bunch for working on this @Falco20019! Here are some comments of the current state — the PR needs a rebase and has some test/doc issues we should fix before merging.
|
@joperezr Thanks for the review. I hope I addressed all remarks. I sadly still have issues on macOS to get it built/tested, so had to make the adjustments untested and hoped for the results here in GH. EDIT: Seems that I'm affected by grpc/grpc#38538 |
joperezr
left a comment
There was a problem hiding this comment.
Thanks for addressing the previous round of feedback! The XML docs and attributes look good now. Just two remaining issues in the tests that are causing the 5 CI failures — both are fallout from the merge with main (the scheme-based key change from #14626). Should be a quick fix!
joperezr
left a comment
There was a problem hiding this comment.
cc @mitchdenny @davidfowl since this is appmodel, can you please take a look at the new API shape?
|
@joperezr Thanks for the feedback. Since #15599 ended up on EDIT: Still not working on macOS... Maybe someone else (able to run tests) could fix them if they are quick ones? I opened a bug ticket #16354 to possibly be able to work on Aspire at some point in time. But right now, even easy things just take forever due to not being able to compile and test, sorry. I gave edit permission to maintainers. |
Still not able to compile locally with macOS
|
I hope to have fixed them by best guessing. |
|
Any update? It's been another 2 weeks without feedback :( As @joperezr mentioned, it should be nearly finished. |
Description
This allows to use
ContainerResourcewithWithReference. Currently the interface enforcesIResourceWithServiceDiscoverywhile internally only using/needingIResourceWithEndpoints. For backwards compatibility I will leave the original ones although redundant.Fixes #10286
Checklist
<remarks />and<code />elements on your triple slash comments?doc-ideatemplatebreaking-changetemplatediagnostictemplate