additional ports support#3612
Merged
Merged
Conversation
vhvb1989
requested review from
ellismg,
hemarina,
wbreza and
weikanglim
as code owners
weikanglim
reviewed
Mar 28, 2024
Contributor
weikanglim
left a comment
weikanglim
left a comment
There was a problem hiding this comment.
Overall looks good. Just brought up some questions if we want to simplify buildAcaIngress -- it's 80+ lines long, and I did find it a little hard to parse through.
ellismg
reviewed
Mar 29, 2024
Member
ellismg
left a comment
ellismg
left a comment
There was a problem hiding this comment.
I do think that this is pretty close (and the detailed tests give me a better understanding of what's going on and confidence this is right, which is great!) - but I will admit that I have sort of forgotten the algorithm that we discussed with David and his C# code sample, so I can't say for sure that I understand exactly what's going on here and can confirm it was correct. I made a few comments about the actual merging of values with things that stuck out or confused me, but I feel like this sort of thing is the place where a few helpful comments as we go along about what we are doing (and what invariants we've established) might be helpful.
Member
|
After testing this, we need to relax some of the rules around how we pick the http ingress. If there are multiple HTTP internal endpoints, the first one becomes the ingress and the rest are additional ports. |
Member
|
I made that change here microsoft/aspire@9077747 |
Member
|
From our meeting, instead of throwing because of the 5 endpoint limitation, we warn but let the deployment go through. This means we don't need to update azd if a customer gets that restriction relaxed. |
ellismg
reviewed
Apr 1, 2024
ellismg
reviewed
Apr 1, 2024
ellismg
reviewed
Apr 1, 2024
Member
ellismg
left a comment
ellismg
left a comment
There was a problem hiding this comment.
Thanks for the improvements to the readability of the core binding algorithm. Overall looks good - but let's adopt something like what I outlined to track the order of the keys instead of a global written to by UnmarshalText. Once that's done, I think this will be good!
Collaborator
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSIDocumentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
weikanglim
approved these changes
Apr 1, 2024
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix: #3267