New WithEndpointsInEnvironment method

[davidebbo]

Fixes #4472

Microsoft Reviewers: Open in CodeFlow

[dotnet-bot]

‼️ Found issues ‼️

Project	Coverage Type	Expected	Actual
Microsoft.Extensions.ServiceDiscovery	Branch	81	80.13 🔻
Microsoft.Extensions.ServiceDiscovery	Line	81	80.94 🔻

Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=722400&view=codecoverage-tab

[mitchdenny]

OK, so I'll reiterate my comment above here that I think that the EnvironmentInjectionFilterEntry type could just be an annotation (which means it could possibly work across resource types more easily as well).

Also we need a live test that covers the endpoint filtering so that the code path in ApplicationExecutor is validated.

[davidebbo]

OK, so I'll reiterate my comment above here that I think that the EnvironmentInjectionFilterEntry type could just be an annotation (which means it could possibly work across resource types more easily as well).

To make sure I understand, what you're suggestion is an implementation change, but not a change to the public API, right? i.e. we would add an annotation for each filter, instead of using alternate data structures. And this annotation would be internal, right?

[mitchdenny]

@davidebbo that's right. The annotation will probably need to be public also.

[davidebbo]

Got it, I'll experiment with that tonight.

…

On Mon, Jul 8, 2024, 3:21 PM Mitch Denny ***@***.***> wrote: @davidebbo <https://github.com/davidebbo> that's right. The annotation will probably need to be public also. — Reply to this email directly, view it on GitHub <#4679 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEHZTQNZPAXJCNHPJASG53ZLKG4JAVCNFSM6AAAAABJ6JZAJWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMJUGA3DCNZUHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[davidebbo]

@mitchdenny I changed it to used annotations to see what that feels like.

Misc comments:

• It fits well into the annotation model, and does make the code a bit simpler.
• The annotation index has no bearing on the behavior. i.e. I did not make it such that a given filter only affects annotations that come before it, as that felt quirky.
• It's a bit less efficient as we need to cycle through all annotations each time we want to test a filter.
• I kept the annotation internal for now, as it has kind of an ugly name, and I'm not clear what scenario we're enabling by making it public. If public, users might try to add it to other resource types, and I think for now, @davidfowl wanted to limit this to Projects. This could easily be revisited later.
• I modified a live executor test to cover this (EndpointPortsProjectNoPortNoTargetPort)

[mitchdenny]

@davidebbo this is looking good. Thanks for this submission. Unless there is anything else you think we should wait for I'd like to get this merged in.

[davidebbo]

@mitchdenny I think it's good to go. Thanks for the review and suggestions!