Allow specifying no default port when building PropertiesEndpointGroup #993
Conversation
This file contains 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
minwoox
approved these changes
Feb 6, 2018
imasahiro
approved these changes
Feb 6, 2018
hyangtack
approved these changes
Feb 6, 2018
| getClass().getClassLoader(), "server-list.properties", "serverA.hosts"); | ||
|
|
||
| assertThat(endpointGroup.endpoints()).containsOnly(Endpoint.of("127.0.0.1:8080"), | ||
| Endpoint.of("127.0.0.1:8081"), |
trustin
force-pushed
the
fix_props_endpoint_groups
branch
from
cd3a30f
to
0b9d663
Compare
trustin
force-pushed
the
fix_props_endpoint_groups
branch
from
0b9d663
to
5d758c4
Compare
|
Removed the 'defect' label because the exception message is actually correct. I made the exception is raised at earlier point for less confusion and correctness. |
Codecov Report
@@ Coverage Diff @@
## master #993 +/- ##
==========================================
- Coverage 72.27% 72.26% -0.02%
==========================================
Files 505 505
Lines 21957 21967 +10
Branches 2683 2684 +1
==========================================
+ Hits 15870 15874 +4
- Misses 4668 4677 +9
+ Partials 1419 1416 -3
Continue to review full report at Codecov.
|
trustin
force-pushed
the
fix_props_endpoint_groups
branch
from
5d758c4
to
5dfc6b3
Compare
Motivation: A user does not always want to specify the default port number when building a PropertiesEndpointGroup, especially for HTTP and HTTPS endpoints that run on their default ports. Modifications: - Add two overloaded factory methods that do not require 'defaultPort' - Fix off-by-one validation bug with 'defaultPort' - Overall clean-up on Javadoc and exception messages Result: - A user can build PropertiesEndpointGroup without defaultPort - Specifying 0 as defaultPort will trigger an exception at earlier point.
trustin
force-pushed
the
fix_props_endpoint_groups
branch
from
5dfc6b3
to
c967d01
Compare
|
Updated the test case to increase the test coverage. |
|
Thanks, reviewers! |
fmguerreiro
pushed a commit
to fmguerreiro/armeria
that referenced
this pull request
Sep 19, 2020
line#993) Motivation: A user does not always want to specify the default port number when building a PropertiesEndpointGroup, especially for HTTP and HTTPS endpoints that run on their default ports. Modifications: - Add two overloaded factory methods that do not require 'defaultPort' - Fix off-by-one validation bug with 'defaultPort' - Overall clean-up on Javadoc and exception messages Result: - A user can build PropertiesEndpointGroup without defaultPort - Specifying 0 as defaultPort will trigger an exception at earlier point.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Motivation:
A user does not always want to specify the default port number when
building a PropertiesEndpointGroup, especially for HTTP and HTTPS
endpoints that run on their default ports.
Modifications:
Result: