Add command line option -server.bindaddress #1102

iils-jschmidt · 2022-06-28T14:13:59Z

This allows to specify the binding address of the fls in addition to the port.
If nothing is specified the wildcard 0.0.0.0 is used, that assumes to listen
on all available addresses.

Usage:
-server.bindaddress=192.168.123.123

This should also work with ipv6 addresses, however I cannot test this at the
moment.

The API Versions in ServerBindAdress.java have to be correct to the actual value.

eparovyshnaya

Dear @JensGW
thank you very much to the handful improvement.

Unfortunately, if you do not have eclipse committer aggreement signed between you and Eclipse Foundation, you won't be able to commit to eclipse repository.

Can I take your contribution and commit it myself?

eparovyshnaya · 2022-06-28T14:46:12Z

...es/org.eclipse.passage.lic.jetty/src/org/eclipse/passage/lic/internal/jetty/JettyServer.java

@@ -33,12 +35,14 @@ public JettyServer(Supplier<JettyHandler> handler) {
 		this.handler = handler;
 	}

-	public void launch(Port port) throws JettyException {
+	public void launch(ServerBindAddress serverBindAddress, Port port) throws JettyException {


we avoid compound variable names wherever we can. This should be named bind.

hm. i would still suggest bindAddress. In contrast to port the word bind feels to unspecific, and only address is to abitrary.

Please, choose any word you like, but if it is a variable or parameter, it must be a single word. Have a look around - there is hardly handful of compound names in the whole project.

I agree on concise naming and being as short as possible. A convention should help guide and not enforce a inferior choice of name. In this specific case the variable name is the same as the command line argument, e.g. -server.port<->port and -server.bindaddress<->bindAddress.

First, the command line argument must not be shortened to -server.address because then the naming is unclear. One could assume it is an address the server should connect to over the network rather then bind to it itself and listen for connections on the binding address.

Then the variable name has to match the command line argument to make it clear that the argument's value ends up in the variable.

Second, just the word bind is out of scope, the variable does not describe a data strucutre that holds a bind to a network interface.

Therefore, please reconsider the name again.

@JensGW

Command line argument is a part of user interface - it stays bindaddress.
Variable name has no obligation to match interface naming conventions: they are oriented to different purposes and pusue totally different goals - user interface must stay clear to an end user, source code must stay maintainable.

In the scope of Passage project, maintainability, besides architectural clarity, states the set of particular very formal rules. Compoud names discouraging is one of them.

Project code convention is not optional.

Maintainability is important. However, a big part of maintaining is understanding the code. Names that are to long or to short make that task more difficult since the meaning has to be inferred from surrounding code instead of just read.
Restricting the naming to single words only greatly diminishes the expressiveness and precision which, in my opinion, makes maintainability more difficult. Having said that, I renamed the marked occurrences to a single word, see updated PR.

eparovyshnaya · 2022-06-28T14:46:32Z

...es/org.eclipse.passage.lic.jetty/src/org/eclipse/passage/lic/internal/jetty/JettyServer.java

 		try {
-			server = Optional.of(new Server(port.get().get()));
+			InetSocketAddress addr = InetSocketAddress.createUnresolved(serverBindAddress.get().get(),


please rename to address

eparovyshnaya · 2022-06-28T14:47:59Z

....passage.lic.jetty/src/org/eclipse/passage/lic/internal/jetty/interaction/ServerHandles.java

 	private final LicenseProtection license = new LicenseProtection();
+	private final ServerBindAddress serverBindAddress;


compound name again

please pay attention to my comment above

iils-jschmidt · 2022-06-28T15:02:00Z

Hi there,
hm, I signed the agreement but it does not seem to work.
Sure you can commit it yourself. Should I change the tings you remarked and update the PR?

ruspl-afed · 2022-06-28T16:15:24Z

Thank you for your effort @JensGW

hm, I signed the agreement but it does not seem to work.

Please open Eclipse profile and specify your GitHub account there, it should do the trick

eparovyshnaya · 2022-06-28T16:33:11Z

...es/org.eclipse.passage.lic.jetty/src/org/eclipse/passage/lic/internal/jetty/JettyServer.java

@@ -33,12 +35,14 @@ public JettyServer(Supplier<JettyHandler> handler) {
 		this.handler = handler;
 	}

-	public void launch(Port port) throws JettyException {
+	public void launch(BindAddress bindAddress, Port port) throws JettyException {


compound names are prohibited in Passage Core with only exception for intentionally badly designed classes (like illustrative implementations for AccessCycleConfiguration)

I agree on concise naming and being as short as possible. A convention should help guide and not enforce a inferior choice of name. In this specific case the variable name is the same as the command line argument, e.g. -server.port<->port and -server.bindaddress<->bindAddress.

First, the command line argument must not be shortened to -server.address because then the naming is unclear. One could assume it is an address the server should connect to over the network rather then bind to it itself and listen for connections on the binding address.

Then the variable name has to match the command line argument to make it clear that the argument's value ends up in the variable.

Second, just the word bind is out of scope, the variable does not describe a data strucutre that holds a bind to a network interface.

Therefore, please reconsider the name again.

please pay attention to my comment above

iils-jschmidt · 2022-06-28T16:41:13Z

Thank you for your effort @JensGW

hm, I signed the agreement but it does not seem to work.

Please open Eclipse profile and specify your GitHub account there, it should do the trick

thx, that did the trick

eparovyshnaya · 2022-06-29T12:23:47Z

...les/org.eclipse.passage.lic.net/src/org/eclipse/passage/lic/internal/net/connect/Listen.java

+/**
+ * @since 2.4
+ */
+public final class Listen extends CliParameter<String> {


cli-param describing class better be named BindAddress

eparovyshnaya · 2022-06-29T12:24:00Z

...les/org.eclipse.passage.lic.net/src/org/eclipse/passage/lic/internal/net/connect/Listen.java

+
+import java.util.Optional;
+
+//FIXME: correct since versions


eparovyshnaya

Two more compound names, all the rest is fine.

And.
As this looks as the first change of the bundle after recent release, piple will not be satisfied until you update service fragment bundle version. 0.1.300 will be fine.

org.eclipse.passage.lic.jetty: Only qualifier changed for (org.eclipse.passage.lic.jetty/0.1.200.v20220629-1247). Expected to have bigger x.y.z than what is available in baseline

eparovyshnaya · 2022-06-29T13:44:05Z

....passage.lic.jetty/src/org/eclipse/passage/lic/internal/jetty/interaction/ServerHandles.java

@@ -62,16 +65,19 @@ public void restart() {

 	public void state() {
 		try {
-			String where = port.get().map(i -> i.toString()).orElse("-"); //$NON-NLS-1$
-			System.out.println(server.state() + " on port " + where); //$NON-NLS-1$
+			String usedBindAddress = listen.get().orElse("-"); //$NON-NLS-1$


compound names again

Here I am running out of names, it is the same thing in a different type, when I cannot compound I have to hide fields. See updated PR.

This allows to specify the binding address of the fls in addition to the port. If nothing is specified the wildcard 0.0.0.0 is used, that assumes to listen on all available addresses. Usage: -server.bindaddress=192.168.123.123 This should also work with ipv6 addresses, however I cannot test this at the moment.

eparovyshnaya · 2022-06-29T14:09:22Z

Thank you for your effort, @JensGW

iils-jschmidt mentioned this pull request Jun 28, 2022

FLS cannot define server bind address #1101

Closed

eparovyshnaya self-requested a review June 28, 2022 14:23

eparovyshnaya reviewed Jun 28, 2022

View reviewed changes

iils-jschmidt force-pushed the master branch 2 times, most recently from 2a5fa37 to 826c5db Compare June 28, 2022 15:18

eparovyshnaya self-requested a review June 28, 2022 16:19

eparovyshnaya requested changes Jun 28, 2022

View reviewed changes

iils-jschmidt force-pushed the master branch from 826c5db to 52aad55 Compare June 29, 2022 10:13

eparovyshnaya requested changes Jun 29, 2022

View reviewed changes

iils-jschmidt force-pushed the master branch from 52aad55 to c9ba9c6 Compare June 29, 2022 12:47

eparovyshnaya approved these changes Jun 29, 2022

View reviewed changes

eparovyshnaya self-requested a review June 29, 2022 13:43

eparovyshnaya requested changes Jun 29, 2022

View reviewed changes

iils-jschmidt force-pushed the master branch from c9ba9c6 to 6654d96 Compare June 29, 2022 13:47

iils-jschmidt force-pushed the master branch from 6654d96 to 15c8f23 Compare June 29, 2022 13:59

eparovyshnaya approved these changes Jun 29, 2022

View reviewed changes

eparovyshnaya merged commit 841a344 into eclipse-passage:master Jun 29, 2022

HannesWell mentioned this pull request Jun 30, 2022

Adapt Passage's Oomph-setup to 2.4.0 release and perform minor version bump #1109

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add command line option -server.bindaddress #1102

Add command line option -server.bindaddress #1102

iils-jschmidt commented Jun 28, 2022

eparovyshnaya left a comment •

edited

Loading

eparovyshnaya Jun 28, 2022

iils-jschmidt Jun 28, 2022

iils-jschmidt Jun 28, 2022

eparovyshnaya Jun 28, 2022

iils-jschmidt Jun 28, 2022

eparovyshnaya Jun 29, 2022 •

edited

Loading

iils-jschmidt Jun 29, 2022

eparovyshnaya Jun 28, 2022

iils-jschmidt Jun 28, 2022

iils-jschmidt Jun 28, 2022

eparovyshnaya Jun 28, 2022

eparovyshnaya Jun 29, 2022

iils-jschmidt commented Jun 28, 2022

ruspl-afed commented Jun 28, 2022

eparovyshnaya Jun 28, 2022

iils-jschmidt Jun 28, 2022

eparovyshnaya Jun 29, 2022

iils-jschmidt commented Jun 28, 2022

eparovyshnaya Jun 29, 2022

eparovyshnaya Jun 29, 2022

eparovyshnaya left a comment

eparovyshnaya Jun 29, 2022

iils-jschmidt Jun 29, 2022

eparovyshnaya commented Jun 29, 2022

		private final LicenseProtection license = new LicenseProtection();
		private final ServerBindAddress serverBindAddress;

Add command line option -server.bindaddress #1102

Add command line option -server.bindaddress #1102

Conversation

iils-jschmidt commented Jun 28, 2022

eparovyshnaya left a comment • edited Loading

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

eparovyshnaya Jun 29, 2022 • edited Loading

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

iils-jschmidt commented Jun 28, 2022

ruspl-afed commented Jun 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iils-jschmidt commented Jun 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eparovyshnaya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eparovyshnaya commented Jun 29, 2022

eparovyshnaya left a comment •

edited

Loading

eparovyshnaya Jun 29, 2022 •

edited

Loading