feat: use real hostnames & auto edit host file #2586

BlairCurrey · 2024-03-19T20:47:51Z

Changes proposed in this pull request

updates all urls to use service-based hostname (e.g. http://cloud-nine-wallet-test-backend:3000)
adds hosts (if not already set) automatically in test/integration/scripts/run-tests.sh

Context

Checklist

netlify · 2024-03-19T20:48:07Z

✅ Deploy Preview for brilliant-pasca-3e80ec canceled.

Name	Link
🔨 Latest commit	`3ab2a58`
🔍 Latest deploy log	https://app.netlify.com/sites/brilliant-pasca-3e80ec/deploys/65fdce77977dd30008fe8624

BlairCurrey · 2024-03-19T20:52:57Z

test/integration/scripts/run-tests.sh

+addHost() {
+  local hostname="$1"
+
+  # check first to avoid sudo prompt if host is already set
+  if hostile list | grep -q "127.0.0.1 $hostname"; then
+    echo "$hostname already set"
+  else
+    sudo hostile set 127.0.0.1 "$hostname"
+  fi
+}


If hosts aren't present you will be prompted for your password so it can write to /etc/hosts. If hosts are present, it will not prompt you. hostile set does nothing if the host is already present but it still prompts, hence the check.

In CI, there is a step before running tests that adds them so we dont need to navigate sudo here in ci. #2569 (comment)

Note that we didnt need sudo-prompt here. Unless I misunderstood the original suggestion, its just not necessary for this purpose. Correct me if I'm missing something though.

Note that we didnt need sudo-prompt here.

So why do we include it?

It's not included. I'm referring to this npm package https://www.npmjs.com/package/sudo-prompt, not the use of sudo here.

BlairCurrey · 2024-03-19T21:00:52Z

With this change you can remove the 127.0.0.1 host.docker.internal entry from /etc/hosts (once merged). Doesn't hurt anything if you don't though.

sabineschaller · 2024-03-20T12:47:37Z

there are still some host.docker.internal in the docker-compose files. I'm assuming that is intentional because those are not referring to the webhook and the exchange rate urls?

BlairCurrey · 2024-03-20T14:03:15Z

there are still some host.docker.internal in the docker-compose files. I'm assuming that is intentional because those are not referring to the webhook and the exchange rate urls?

Good catch, should have pointed that out. Yes, that's exactly right. That's so our backend services can reach the test's server on the hostmachine.

On that note, I removed the extra_hosts: host.docker.interal but maybe I should return it. It works without for me but @mkurapov suggested it may be necessary for linux, if I recalled correctly. I will read up on that a bit and probably just return it to be safe.

EDIT: Actually I did not remove them. 😁 Guess I just intended to at one point.

sabineschaller · 2024-03-22T07:26:46Z

test/integration/lib/integration-server.ts

-    this.server = this.app.listen(port, () => {
-      console.log(`Integration server listening on port ${port}`)
-    })
+    this.server = this.app.listen(port)


Is removing thing console.log part of us not wanting to console.log things anymore but wanting to do proper logging in the future?

Somewhat I guess - I added it initially when I was debugging service to this test server requests.

On that note I don think using pino logger here or anywhere else that is dev only would have any benefit. They aren't run with our other services and aren't going to be consumed by some sort of log aggregator or cloud logging viewer. Just development.

I'm mainly concerned about readability which pino-pretty would largely resolve. Jest has it's own "prettyfied" logs though so if using pino-pretty we should not pipe it and instead add it in code to preserve jest's output formatting.

I think there is 1 remaining "unknown webhook event" (or similar) log in the tests here FWIW.

sabineschaller · 2024-03-22T07:27:38Z

test/integration/scripts/run-tests.sh

+addHost() {
+  local hostname="$1"
+
+  # check first to avoid sudo prompt if host is already set
+  if hostile list | grep -q "127.0.0.1 $hostname"; then
+    echo "$hostname already set"
+  else
+    sudo hostile set 127.0.0.1 "$hostname"
+  fi
+}


Note that we didnt need sudo-prompt here.

So why do we include it?

mkurapov · 2024-03-22T10:25:02Z

@BlairCurrey I think we only need to keep

    extra_hosts:
      - 'host.docker.internal:host-gateway'

in the backend services (I don't think auth needs to call the host machine)

mkurapov · 2024-03-22T10:12:54Z

test/integration/scripts/run-tests.sh

+  if hostile list | grep -q "127.0.0.1 $hostname"; then
+    echo "$hostname already set"
+  else
+    sudo hostile set 127.0.0.1 "$hostname"


Suggested change

if hostile list | grep -q "127.0.0.1 $hostname"; then

echo "$hostname already set"

else

sudo hostile set 127.0.0.1 "$hostname"

if pnpm hostile list | grep -q "127.0.0.1 $hostname"; then

echo "$hostname already set"

else

sudo pnpm hostile set 127.0.0.1 "$hostname"

right? or did I not install it correctly?

pnpm i is all you need to do and I can reproduce this now. Looks like we're calling it differently.

I was running with pnpm --filter integration run-tests (added after moving to run-tests.sh to ./test/integration/scripts). But if it can be called directly that would be good too. If I make your changes I can only get it to work if running ./scripts/run-test.sh from the context of ./test/integration.

Adding a pnpm command and using pnpm --filter has it working in all contexts for me now (root using pnpm --filter, calling script directly from root, calling script directly from package). ec8d5ab

BlairCurrey · 2024-03-22T17:55:23Z

For adding hosts in CI, we can't just remove the Setup hosts step.

Just removing the setup host step fails because root doesn't find pnpm:

sudo: pnpm: command not found

https://github.com/interledger/rafiki/actions/runs/8392609267/job/22985668877?pr=2586

Doesn't happen locally, and I can only conclude this is because of differences in installing (npm/corepack vs. pnpm/action-setup).

I see these options:

keeping the setup host step
removing the setup host step and passing in a pnpm binary path to the script for us in CI? like pnpm --filer integration run-tests -p /home/runner/setup-pnpm/node_modules/.bin/pnpm or maybe just -p $(which pnpm)
removing the setup host step and adding a step to add pnpm to the binary path somehow

At this point I think sticking with 1 makes sense. No matter what we do this isn't going to "just work" without either passing in the path or doing some extra setup. Other than that, 3 seemed like the obvious fix but I made some attempts at adding pnpm to the root path but did not get it working (82678cd, 645ef16, 9e24ebb). I believe 645ef16 in particular should have correctly set it but the pnpm command still was not found. Thus the option to pass it in, which should work because here it is working with the ci path hardcoded: 5302a0f.

mkurapov · 2024-03-27T16:11:21Z

Keeping the setup host step in CI is the most straightforward solution.

BlairCurrey added 4 commits March 19, 2024 15:47

feat: auto edit host file

07ec4fd

chore: rm debug log

53c96ff

feat: use real hostnames

91ca75a

fix: add auth hostnames

a1c4053

BlairCurrey commented Mar 19, 2024

View reviewed changes

BlairCurrey marked this pull request as ready for review March 19, 2024 20:57

BlairCurrey requested review from mkurapov, raducristianpopa, sabineschaller and njlie March 19, 2024 20:57

chore: update comment w/ side effect

4191706

sabineschaller reviewed Mar 22, 2024

View reviewed changes

mkurapov reviewed Mar 22, 2024

View reviewed changes

BlairCurrey added 4 commits March 22, 2024 10:05

chore: rm unused compose option

6337cf5

fix: hostile cmd not found when script called directly from package

ec8d5ab

Merge branch 'main' into bc/2555/hostnames-integration-test

d145374

chore: rm step to setup hosts, lets run-test do it

dcd6082

github-actions bot added the type: ci Changes to the CI label Mar 22, 2024

BlairCurrey added 7 commits March 22, 2024 11:53

fix(integration): dont run tests if hostfile write fails

aaad52e

fix(integration): re-comment out comment

fa42122

chore: gh action debug

124c847

chore: maybe make pnpm available to root?

622f49e

chore: maybe fix adding pnpm to path in ci

d97fd78

chore: maybe fix adding pnpm to root path

e2a8290

chore: rm debug code

3fb89de

BlairCurrey added 2 commits March 22, 2024 13:06

chore: call hostile directly, no pnpm

8d3095d

chore: debug

5302a0f

chore(integration): restore setup hosts step

76c5c9c

github-actions bot removed the type: ci Changes to the CI label Mar 22, 2024

BlairCurrey added 2 commits March 22, 2024 13:57

chore(integration): rm unused var

d05a878

fix(integration): add real hostnames

5b8847a

github-actions bot added the type: ci Changes to the CI label Mar 22, 2024

BlairCurrey added 6 commits March 22, 2024 14:06

fix(integration): how items are written to /etc/hosts

ff49f7d

chore: debug pnpm sudo path

82678cd

chore: rm old dev bruno requests

a5afe9e

chore: ensure env var set in correct step

645ef16

chore(integration): debugging

9e24ebb

chore: restore setup hosts step

3ab2a58

BlairCurrey requested review from mkurapov and sabineschaller March 22, 2024 18:40

mkurapov approved these changes Mar 27, 2024

View reviewed changes

BlairCurrey merged commit 78b5574 into main Mar 27, 2024
23 checks passed

BlairCurrey deleted the bc/2555/hostnames-integration-test branch March 27, 2024 16:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: use real hostnames & auto edit host file #2586

feat: use real hostnames & auto edit host file #2586

BlairCurrey commented Mar 19, 2024 •

edited

Loading

netlify bot commented Mar 19, 2024 •

edited

Loading

BlairCurrey Mar 19, 2024 •

edited

Loading

sabineschaller Mar 22, 2024

BlairCurrey Mar 22, 2024 •

edited

Loading

BlairCurrey commented Mar 19, 2024 •

edited

Loading

sabineschaller commented Mar 20, 2024

BlairCurrey commented Mar 20, 2024 •

edited

Loading

sabineschaller Mar 22, 2024

BlairCurrey Mar 22, 2024

BlairCurrey Mar 22, 2024 •

edited

Loading

sabineschaller Mar 22, 2024

mkurapov commented Mar 22, 2024 •

edited

Loading

mkurapov Mar 22, 2024

BlairCurrey Mar 22, 2024 •

edited

Loading

BlairCurrey commented Mar 22, 2024 •

edited

Loading

mkurapov commented Mar 27, 2024

feat: use real hostnames & auto edit host file #2586

feat: use real hostnames & auto edit host file #2586

Conversation

BlairCurrey commented Mar 19, 2024 • edited Loading

Changes proposed in this pull request

Context

Checklist

netlify bot commented Mar 19, 2024 • edited Loading

✅ Deploy Preview for brilliant-pasca-3e80ec canceled.

BlairCurrey Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

sabineschaller Mar 22, 2024

Choose a reason for hiding this comment

BlairCurrey Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

BlairCurrey commented Mar 19, 2024 • edited Loading

sabineschaller commented Mar 20, 2024

BlairCurrey commented Mar 20, 2024 • edited Loading

sabineschaller Mar 22, 2024

Choose a reason for hiding this comment

BlairCurrey Mar 22, 2024

Choose a reason for hiding this comment

BlairCurrey Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

sabineschaller Mar 22, 2024

Choose a reason for hiding this comment

mkurapov commented Mar 22, 2024 • edited Loading

mkurapov Mar 22, 2024

Choose a reason for hiding this comment

BlairCurrey Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

BlairCurrey commented Mar 22, 2024 • edited Loading

mkurapov commented Mar 27, 2024

BlairCurrey commented Mar 19, 2024 •

edited

Loading

netlify bot commented Mar 19, 2024 •

edited

Loading

BlairCurrey Mar 19, 2024 •

edited

Loading

BlairCurrey Mar 22, 2024 •

edited

Loading

BlairCurrey commented Mar 19, 2024 •

edited

Loading

BlairCurrey commented Mar 20, 2024 •

edited

Loading

BlairCurrey Mar 22, 2024 •

edited

Loading

mkurapov commented Mar 22, 2024 •

edited

Loading

BlairCurrey Mar 22, 2024 •

edited

Loading

BlairCurrey commented Mar 22, 2024 •

edited

Loading