Port manager #42

camden11 · 2023-09-29T18:37:37Z

Description and Context

See https://git.hubteam.com/HubSpot/hubspot-cli-issues/issues/462

This PR is a proof of concept for the Port Manager Server, which seeks to address the above issue by creating a separate service that the CLI interfaces to assign ports to dev servers, assuring that no port conflicts happen when running multiple servers. The Port Manager Server also provides an API for the UIExtensionManager (and future extension managers) to interface with from the UI, telling it which ports to look for dev servers on.

The bulk of the functionality of this PR is in the following three files

utils/PortManagerServer.ts - Class that handles spinning the the express server
utils/detectPort.ts - Logic for assigning ports based on what's available
lib/portManager.ts - set of functions that the CLI (and other repos, potentially) can use to easily interact with the port manager.

Related port manager PRs:
HubSpot/hubspot-cli#936
https://git.hubteam.com/HubSpot/ui-extensibility/pull/1566

TODO

Figure out what port to run the port manager on

Who to Notify

@brandenrodgers @kemmerle

brandenrodgers · 2023-10-16T20:23:45Z

constants/ports.ts

+export const MAX_PORT_NUMBER = 65535;
+
+// TODO: What's a good port for the port manager?
+export const PORT_MANAGER_SERVER_PORT = 12345;


Maybe 8080? That's a classic one. Maybe we could also make it configurable?

8080 is good. Making it configurable could be tricky because the UI needs to know where to look for it

brandenrodgers · 2023-10-16T20:27:51Z

utils/PortManagerServer.ts

+    this.app.use(express.json());
+    this.app.use(cors());
+    this.setupRoutes();
+    this.server = this.app.listen(PORT_MANAGER_SERVER_PORT, () =>


You may want to pull some of the logic from here that forces the rest of the process to wait to make sure that the server started up successfully. It makes it easier to catch + log errors in the calling function

brandenrodgers · 2023-10-16T20:30:02Z

utils/PortManagerServer.ts

+    this.app.post('/close', this.closeServer);
+  }
+
+  setPort(instanceId: string, port: number) {


Should this protect against initializing duplicate instanceId's?

setPort is only called by assignPortToServer, which does protect against duplicates

brandenrodgers · 2023-11-07T14:59:32Z

constants/ports.ts

+export const MIN_PORT_NUMBER = 1024;
+export const MAX_PORT_NUMBER = 65535;
+
+// TODO: What's a good port for the port manager?


lingering todo (unless you still plan to revisit this)

brandenrodgers · 2023-11-07T15:00:15Z

lang/en.lyaml

@@ -136,6 +136,12 @@ en:
    processFieldsJs:
      converting: 'Converting "{{ src }}" to "{{ dest }}".'
      converted: 'Finished converting "{{ src }}" to "{{ dest }}".'
+    utils:


I'm realizing we're going to hit some merge conflicts with all of the WIP PR's and my translations spike PR

brandenrodgers · 2023-11-07T15:01:02Z

lang/en.lyaml

+        started: "PortManagerServer running on port {{ port }}"
+        setPort: "Server with instanceId {{ instanceId }} assigned to port {{ port }}"
+        deletedPort: "Server with instanceId {{ instanceId }} unassigned from port {{ port }}"
+        close: "PortManagerServer shutting down."


I wonder if we should refer to this differently in logs. End users probably don't need to know about portManagerServer

That's true, though these are all debug logs so theoretically end users wouldn't be seeing them. Seemed helpful if anyone was ever debugging something related to this or the inner workings became relevant to their workflow

Ah ok, I was thinking that they were standard logs. That makes sense 👌

brandenrodgers · 2023-11-07T15:08:46Z

utils/detectPort.ts

+
+export function detectPort(port?: number | null): Promise<number> {
+  if (port && (port < MIN_PORT_NUMBER || port > MAX_PORT_NUMBER)) {
+    throw new Error('Port must be between 1024 and 65535');


We should put this copy in the lang file

Ah good call, I guess I'll hold off on that until the lang stuff is merged

brandenrodgers · 2023-11-08T15:54:48Z

utils/PortManagerServer.ts

+
+const i18nKey = 'utils.PortManagerServer';
+
+class PortManagerServer {


Not a blocker - I wonder if we should even bother having a models/ folder in this repo. I think we have a few other classes in here that also aren't in that folder.

Yeah 🤷 . Was left over from cli-lib but definitely don't think we need to make the distinction. I'm down to remove in the future

brandenrodgers · 2023-11-08T15:59:03Z

utils/PortManagerServer.ts

+    });
+  };
+
+  getServerPortByInstanceId = (req: Request, res: Response): void => {


Should the return on this be Promise?

Don't thinks so, this isn't async. Express knows how to handle it as is

brandenrodgers · 2023-11-08T16:01:18Z

utils/detectPort.ts

@@ -0,0 +1,90 @@
+import net, { AddressInfo } from 'net';


We may need to include the copyright info in here per the license: https://github.com/node-modules/detect-port/blob/master/LICENSE

Good call. I could have sworn I had already done this

brandenrodgers · 2023-11-08T16:02:40Z

I'm ok with it being a fast-follow, but we should probably also get some unit tests in place for these new utils. This will be a good start to us getting in the habit of making sure everything new is tested.

brandenrodgers · 2023-11-08T16:04:03Z

lang/en.json

+    },
+    "detectPort": {
+      "errors": {
+        "invalidPort": "Port must be between 1024 and 65535"


It might be useful to have these min and max numbers be dynamic so changing the variable values will automatically change the message here.

brandenrodgers · 2023-11-08T16:04:17Z

lang/en.json

+        "duplicateInstance": "Failed to start PortManagerServer. An instance of PortManagerServer is already running.",
+        "404": "Could not find a server with instanceId {{ instanceId }}",
+        "409": "Failed to assign port. Server with instanceId {{ instanceId }} is already running on port {{ port }}",
+        "400": "Invalid port requested. Port must be between 1024 and 65535."


Same dynamic suggestion here

camden11 added 12 commits September 26, 2023 12:14

start setting up port manager

bcd10b7

detectPort first pass

d028563

Fix port conflict bug

82cd169

Port manager get/set functionality

ecc19c8

add delete endpoint + clean some things up

9b660ea

port manager module first pass

e151907

export port manager

75726b0

rename some things and add server index route

5766ea7

Fix some portmanager bugs

c2e3013

Fix some port manager bugs

060b9f6

use cors

d5ce5cd

Add logging/errors to PortManagerServer

61d81d4

camden11 changed the title ~~WIP: Port manager~~ POC: Port manager Oct 4, 2023

camden11 requested review from kemmerle and brandenrodgers October 4, 2023 18:12

camden11 marked this pull request as ready for review October 4, 2023 18:13

camden11 mentioned this pull request Oct 4, 2023

Use port manager HubSpot/hubspot-cli#936

Merged

brandenrodgers reviewed Oct 16, 2023

View reviewed changes

camden11 added 8 commits October 18, 2023 13:14

Merge branch 'main' into port-manager

b83750f

Wait for PortManagerServer to start

73d05b1

rename assignPortToServerInstance

90ae978

Set up port manager for requesting multiple ports

e79df2a

fix bug in requestPort

1d30147

Merge branch 'main' into port-manager

91a3228

Update request ports to allow for specifying ports

0bb5532

port manager api tweaks

d191ff3

brandenrodgers reviewed Nov 7, 2023

View reviewed changes

camden11 added 5 commits November 7, 2023 15:54

Merge branch 'main' into port-manager

053dba6

remove todo

872304f

restore portmanager types file

2726eec

Merge branch 'main' into port-manager

de09bad

Add detectPort copy to lang file

25eb5ea

camden11 changed the title ~~POC: Port manager~~ Port manager Nov 8, 2023

brandenrodgers reviewed Nov 8, 2023

View reviewed changes

camden11 added 2 commits November 8, 2023 11:19

Add detect port license

825d86b

update invalid port copy

bd1e9cb

brandenrodgers approved these changes Nov 8, 2023

View reviewed changes

camden11 merged commit 7faaeb2 into main Nov 9, 2023
1 check passed

brandenrodgers mentioned this pull request Jan 30, 2024

Add preview command support HubSpot/cli-lib#21

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port manager #42

Port manager #42

camden11 commented Sep 29, 2023 •

edited

Loading

brandenrodgers Oct 16, 2023

camden11 Oct 18, 2023

brandenrodgers Oct 16, 2023

brandenrodgers Oct 16, 2023

camden11 Oct 18, 2023

brandenrodgers Nov 7, 2023

brandenrodgers Nov 7, 2023

brandenrodgers Nov 7, 2023

camden11 Nov 7, 2023

brandenrodgers Nov 8, 2023

brandenrodgers Nov 7, 2023

camden11 Nov 7, 2023

brandenrodgers Nov 8, 2023

camden11 Nov 8, 2023

brandenrodgers Nov 8, 2023

camden11 Nov 8, 2023

brandenrodgers Nov 8, 2023

camden11 Nov 8, 2023

brandenrodgers commented Nov 8, 2023

brandenrodgers Nov 8, 2023

brandenrodgers Nov 8, 2023


		const i18nKey = 'utils.PortManagerServer';

		class PortManagerServer {

Port manager #42

Port manager #42

Conversation

camden11 commented Sep 29, 2023 • edited Loading

Description and Context

TODO

Who to Notify

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandenrodgers commented Nov 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camden11 commented Sep 29, 2023 •

edited

Loading