Skip to content

chore: Replace mockserver with nginx - #8870

Merged
nydr merged 1 commit into
masterfrom
replace-mockserver
Feb 20, 2024
Merged

chore: Replace mockserver with nginx#8870
nydr merged 1 commit into
masterfrom
replace-mockserver

Conversation

@nydr

@nydr nydr commented Feb 14, 2024

Copy link
Copy Markdown
Contributor

Description

Replaces the mockserver used in some tests with a slim nginx image.

This is primarily to reduce the image size required for mocking these tests from 120mb to 5mb but will also slightly reduce resource requirements for these tests.

Code review checklist

  • Readable: Concise, well named, follows the style guide, documented if necessary.
  • Documented: Configuration and user documentation on cht-docs
  • Tested: Unit and/or e2e where appropriate
  • Internationalised: All user facing text
  • Backwards compatible: Works with existing data and configuration or includes a migration. Any breaking changes documented in the release notes.

Compose URLs

If Build CI hasn't passed, these may 404:

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

@nydr nydr changed the title Replace mockserver with nginx chore: Replace mockserver with nginx Feb 14, 2024
@nydr
nydr requested review from Hareet and mrjones-plip February 14, 2024 10:27

@mrjones-plip mrjones-plip left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nydr - nice work to reduce image size and get some more performance out of our tests!

I'm not quite sure how to review this, can you give me some more guidance?

  • I tried running make test on both master and replace-mockserver and they look the same. Good I guess? There are some errors (eg 2 failed, 11 passed in 1.46s) - but maybe since they're the same that's OK?
  • docker compose up works, but GET request to /, /_membership and /error/drop don't work as I'd expect (up is returned in a browser, and nothing in curl). However, this behaivor is the same in master and replace-mockserver - so maybe OK?

@nydr

nydr commented Feb 19, 2024

Copy link
Copy Markdown
Contributor Author

@mrjones-plip Thanks for checking on this! There's three instances where it's being used:

### 1) Nginx
# Should work exactly as before, except the name of the container is changed
# to clarify it's a mocked api, to test:cd nginx/tests/ && make test

### 2) Haproxy
# Here I also increased the number of mock servers from 1 to 3 so it'll take
# clustered couchdb into account, to test:cd haproxy/tests/ && make test

### 3) Haproxy-healthcheck
# Should work exactly as before, not used in any automated tests but used for
# testing haproxy-healthcheck during developmentcd haproxy-healthcheck
❯ docker-compose up

❯ curl localhost:5984/_membership
{ "all_nodes": [ "mock-couchdb" ], "cluster_nodes": [ "mock-couchdb" ] }

❯ nc localhost 5555
up
  • I tried running make test on both master and replace-mockserver and they look the same. Good I guess? There are some errors (eg 2 failed, 11 passed in 1.46s) - but maybe since they're the same that's OK?

Which tests are failing? There was some issues I didn't see on my machine or CI last time #8446 (comment) and while not directly related to this PR I think it's worth investigating further

  • docker compose up works, but GET request to /, /_membership and /error/drop don't work as I'd expect (up is returned in a browser, and nothing in curl). However, this behaivor is the same in master and replace-mockserver - so maybe OK?

I'm not sure I follow, could you walk me through how you're using curl? haproxy-healthcheck is the only compose file which exposes the mock server port and there I'm seeing this:

❯ curl -w "\n" localhost:5984/{,_membership,error/drop}
{ "couchdb": "Welcome to mock-couchdb", "status": "this field is not used" }
{ "all_nodes": [ "mock-couchdb" ], "cluster_nodes": [ "mock-couchdb" ] }
curl: (52) Empty reply from server

up might be coming from the healthcheck itself, (port 5555) and that is a plain tcp server (not http)

❯ nc -v localhost 5555
Connection to localhost port 5555 [tcp/personal-agent] succeeded!
up

@mrjones-plip mrjones-plip left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nydr - thanks for the detailed notes! This is helpful. The main thing I was missing was (embarrassingly) specifying the port 5984. With your updated steps and on your replace-mockserver branch, the make test look good for nginx/test/ and haproxy/test and now the docker compose up call followed by curl does work. nc also works as expected (you guessed right!)

on the master branch the curl call fails but nc succeeds:

➜  haproxy-healthcheck git:(master) curl localhost:5984/_membership

curl: (7) Failed to connect to localhost port 5984: Connection refused
➜  haproxy-healthcheck git:(master) nc localhost 5555
up

That OK? I'll assume use and approve to unblock!

@nydr

nydr commented Feb 20, 2024

Copy link
Copy Markdown
Contributor Author

Thank you for the quick response!

on the master branch the curl call fails but nc succeeds
That OK? I'll assume use and approve to unblock!

Yeah that's fine, on master there was no port forwarding configured for that service in the compose file so it's expected behaviour. Additionally that service is only intended to be used for ad-hoc tests during development of haproxy-healthcheck which will hopefully be sunset at some point in the future.

@nydr
nydr merged commit b7d0052 into master Feb 20, 2024
@nydr
nydr deleted the replace-mockserver branch February 20, 2024 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants