Deprecate ArmeriaSettings.ip and add ArmeriaSettings.address #4598

kojilin · 2022-12-30T04:47:23Z

Motivation:

Spring allows Users to specify a hostname because Spring converts the input to an InetAddress.
So we need to also allow users to specify a hostname.

Modifications:

adding ArmeriaSettings.address whose type is InetAddress
deprecating ArmeriaSettings.ip

Result:

Closes Deprecate ArmeriaSettings.ip and add ArmeriaSettings.address #4597

ikhoon

Thanks @kojilin for the quick fix! 🙇‍♂️

ikhoon · 2022-12-30T05:21:37Z

spring/boot2-autoconfigure/src/main/java/com/linecorp/armeria/spring/ArmeriaSettings.java

+        public void setAddress(InetAddress address) {
+            this.address = address;
+        }


Should return this like other setters?

ikhoon · 2022-12-30T05:23:24Z

spring/boot2-autoconfigure/src/test/java/com/linecorp/armeria/spring/DeprecatedIpTest.java

+        final Collection<ServerPort> serverPorts = server.activePorts().values();
+        for (ServerPort sp : serverPorts) {
+            final InetAddress address = sp.localAddress().getAddress();
+            Assertions.assertThat(address.isAnyLocalAddress() || address.isLoopbackAddress()).isTrue();


nit: Prefer static import for assertions.

Suggested change

Assertions.assertThat(address.isAnyLocalAddress() || address.isLoopbackAddress()).isTrue();

assertThat(address.isAnyLocalAddress() || address.isLoopbackAddress()).isTrue();

jrhee17

Looks really nice! Thanks @kojilin ! 🙇 👍 🙇

jrhee17 · 2023-01-03T03:51:46Z

spring/boot2-autoconfigure/src/main/java/com/linecorp/armeria/spring/ArmeriaSettings.java

+         * Network address to bind to. If not set, will bind to all addresses, e.g. {@code 0.0.0.0}.
+         */
+        @Nullable
+        private InetAddress address;


nit; do you mind also updating the example?

armeria/spring/boot2-autoconfigure/src/main/java/com/linecorp/armeria/spring/ArmeriaSettings.java

Line 51 in e376b8c

* - ip: 127.0.0.1

jrhee17 · 2023-01-03T04:02:14Z

spring/boot2-autoconfigure/src/test/resources/config/application-deprecatedIpTest.yml

@@ -0,0 +1,8 @@
+armeria:
+  ports:
+    - ip: 127.0.0.1


optional; I'm unsure if this would also work in windows, but I think it would be cool if we can check that non-ip addresses are also accepted

Suggested change

- ip: 127.0.0.1

- address: localhost

jrhee17 · 2023-01-03T04:02:45Z

spring/boot2-autoconfigure/src/test/java/com/linecorp/armeria/spring/DeprecatedIpTest.java

@@ -0,0 +1,61 @@
+/*
+ * Copyright 2020 LINE Corporation


Suggested change

* Copyright 2020 LINE Corporation

* Copyright 2023 LINE Corporation

minwoox

Thanks a lot for adding the address. 😄

minwoox · 2023-01-03T08:59:28Z

spring/boot2-autoconfigure/src/main/java/com/linecorp/armeria/spring/ArmeriaSettings.java

@@ -82,10 +83,18 @@ public class ArmeriaSettings {
    public static class Port {
        /**
         * IP address to bind to. If not set, will bind to all addresses, e.g. {@code 0.0.0.0}.
+         * @deprecated Use {@link #address} instead.


Let's add a blank line between the doc and @deprecated. 😉

minwoox · 2023-01-03T08:59:55Z

spring/boot2-autoconfigure/src/main/java/com/linecorp/armeria/spring/ArmeriaSettings.java

        public Port setIp(String ip) {
            this.ip = ip;
            return this;
        }

+        /**
+         * Returns the Network address that the {@link Server} uses.


nit: I guess we don't need to capitalize the word

Suggested change

* Returns the Network address that the {@link Server} uses.

* Returns the network address that the {@link Server} uses.

minwoox · 2023-01-03T09:00:24Z

spring/boot2-autoconfigure/src/test/java/com/linecorp/armeria/spring/DeprecatedIpTest.java

@@ -0,0 +1,61 @@
+/*
+ * Copyright 2023 LINE Corporation


Happy new year. 🎉 🎉 🎉

minwoox · 2023-01-03T09:12:16Z

spring/boot2-autoconfigure/src/test/java/com/linecorp/armeria/spring/DeprecatedIpTest.java

+        final Collection<ServerPort> serverPorts = server.activePorts().values();
+        for (ServerPort sp : serverPorts) {
+            final InetAddress address = sp.localAddress().getAddress();
+            assertThat(address.isAnyLocalAddress() || address.isLoopbackAddress()).isTrue();


Question: are these values different depending on the OS?

not sure, I just put 127.0.0.1 & 0.0.0.0 at yml, so keep test both

Ah ha, if so how about branching it so that we can be sure what the test actually means?

if ("127.0.0.1".equals(address.getHostAddress())) { assertThat(...).isTrue(); } else { // 0.0.0.0 assertThat(...).isTrue(); }

minwoox · 2023-01-03T09:13:33Z

...onfigure/src/main/java/com/linecorp/armeria/internal/spring/ArmeriaConfigurationNetUtil.java

+        final String ip = p.getIp();
+        final int port = p.getPort();
+        if (ip != null && address != null) {
+            throw new IllegalStateException("A port cannot have both IP and address: " + p);


Should we override Port.toString() to properly create the message?

minwoox

Just left one question. Thanks a lot, @kojilin! 😄

minwoox · 2023-01-11T05:52:30Z

spring/boot2-autoconfigure/src/main/java/com/linecorp/armeria/spring/ArmeriaSettings.java

+            return "Port{" +
+                   "ip='" + ip + '\'' +
+                   ", address=" + address +
+                   ", iface='" + iface + '\'' +


question: It seems like we add '' to only ip and iface. Is there any specific reason for that?

I used Intellij to generate and looks like it adds ' or not by String or not.
Maybe I should use MoreObjects.toStringHelper like others?

Didn't know about it. 😄 It looks good now. Thanks!

minwoox · 2023-01-12T09:45:12Z

Thanks a lot, @kojilin! 😄

Deprecate ArmeriaSettings.ip and add ArmeriaSettings.address

22f354e

kojilin requested review from ikhoon, jrhee17, minwoox and trustin as code owners December 30, 2022 04:47

kojilin added 2 commits December 30, 2022 13:49

Using address instead of ip

369b08f

nit

d4af03d

ikhoon approved these changes Dec 30, 2022

View reviewed changes

ikhoon added the new feature label Dec 30, 2022

ikhoon added this to the 1.22.0 milestone Dec 30, 2022

kojilin added 3 commits December 30, 2022 14:33

nit

d64553c

lint

14f2149

Fix code

fef7903

jrhee17 approved these changes Jan 3, 2023

View reviewed changes

Add localhost test

641077b

minwoox reviewed Jan 3, 2023

View reviewed changes

kojilin added 2 commits January 3, 2023 18:24

nit

1609c08

Change test

e19e1e6

minwoox approved these changes Jan 11, 2023

View reviewed changes

Change toString implementation

f4a6785

minwoox merged commit 76c50fe into line:master Jan 12, 2023

kojilin deleted the 4597 branch June 22, 2024 11:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate ArmeriaSettings.ip and add ArmeriaSettings.address #4598

Deprecate ArmeriaSettings.ip and add ArmeriaSettings.address #4598

kojilin commented Dec 30, 2022

ikhoon left a comment

ikhoon Dec 30, 2022

ikhoon Dec 30, 2022

jrhee17 left a comment

jrhee17 Jan 3, 2023

jrhee17 Jan 3, 2023

jrhee17 Jan 3, 2023

minwoox left a comment

minwoox Jan 3, 2023

minwoox Jan 3, 2023

minwoox Jan 3, 2023

minwoox Jan 3, 2023

kojilin Jan 3, 2023

minwoox Jan 3, 2023

minwoox Jan 3, 2023

minwoox left a comment

minwoox Jan 11, 2023

kojilin Jan 12, 2023

minwoox Jan 12, 2023

minwoox commented Jan 12, 2023

	Assertions.assertThat(address.isAnyLocalAddress() \|\| address.isLoopbackAddress()).isTrue();
	assertThat(address.isAnyLocalAddress() \|\| address.isLoopbackAddress()).isTrue();

	* Copyright 2020 LINE Corporation
	* Copyright 2023 LINE Corporation

	* Returns the Network address that the {@link Server} uses.
	* Returns the network address that the {@link Server} uses.

Deprecate ArmeriaSettings.ip and add ArmeriaSettings.address #4598

Deprecate ArmeriaSettings.ip and add ArmeriaSettings.address #4598

Conversation

kojilin commented Dec 30, 2022

ikhoon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox commented Jan 12, 2023