-
Notifications
You must be signed in to change notification settings - Fork 245
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: introduce PortMappingRegistry service #4677
refactor: introduce PortMappingRegistry service #4677
Conversation
450e40d
to
551a4bd
Compare
551a4bd
to
53511b7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a reminder, please update the doc to reflect this.
will to after merge, I am already preparing the change |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One other nit: the name PortMappings
implies just a container object. Could you change it to PortMappingRegistry
or PortMappingService
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹
What this PR changes/adds
Following up #4671 :
Adds a
PortMappingRegistry
service, to centralize thePortMapping
registration.Makes use of
@Configuration
and@Settings
annotation to define api configuration, makingWebServiceConfigurer
not needed (it has been deprecated)Why it does that
refactor
Further notes
JettyService
start method a little, the mappings validations have been moved onPortMappings
(so the runtime would crash earlier in case of inconsistencies, improved methods naming and scopeLinked Issue(s)
Part of #4669
Please be sure to take a look at the contributing guidelines and our etiquette for pull requests.