Support configurable dynamic port range #11167
Conversation
|
Hi @a-zagaevskiy and thanks so much for this. I have marked this for our next major release meaning we won't review this immediately, but will perform this once that development cycle is open to merging into main. |
|
Hi @jrasell ! No problem ! |
Also add a little more min/max port testing and add the consts back that had been removed: but unexported and as defaults.
|
@a-zagaevskiy Thanks for the contribution! I added the @jrasell Mind giving this a review? |
|
@schmichael It looks great! Thank you very much! |
|
I'm still cleaning up client tests locally. |
- Fix test broken due to being improperly setup. - Include min/max ports in default client config.
|
@jrasell Tests are happy! Ready for a review. |
There was a problem hiding this comment.
Hi @a-zagaevskiy and thanks so much for raising this PR. It looks great, however, the one inline comment I made feels like a blocker at the moment. Please let me know if you have any questions, or whether you want me to take over this additional change due to constraints on your end.
There was a problem hiding this comment.
LGTM, thanks a lot @a-zagaevskiy! @schmichael any last thoughts before merging this in from you particularly around the plan applier?
Thanks for reminding me! I confirmed both the scheduler worker and plan applier use the same NetworkIndex code that @a-zagaevskiy altered to support these new configuration parameters. All that to say: it looks good to me! I expanded the error messages and added some more tests and will merge! |
changelog: fixup entry extension for #11167
|
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This changes make it possible to configure the range that is used for assigning dynamic ports (#8186).