Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(smoketests) don't expose companion container ports #1179

Merged

Conversation

arukiidou
Copy link
Contributor

following up at #149

and #1178

The difference is that, the above is a "free" fix for the document only,

this is fix for the test, so it's possible that it could break the test

@ggrossetie
Copy link
Member

@arukiidou Could you please rebase? I just merged #1178

@ggrossetie
Copy link
Member

@arukiidou smoke tests are failing because we are using wait-for-it.sh on port 8001, 8002, 8003 and 8004 in order to make sure that all companion containers are up and running before executing the tests suite.

kroki/Makefile

Lines 74 to 78 in eabc978

&& "$(SMOKE_TESTS_DIR)/wait-for-it.sh" localhost:8000 --timeout="$(COMPOSE_TIMEOUT)" \
&& "$(SMOKE_TESTS_DIR)/wait-for-it.sh" localhost:8001 --timeout="$(COMPOSE_TIMEOUT)" \
&& "$(SMOKE_TESTS_DIR)/wait-for-it.sh" localhost:8002 --timeout="$(COMPOSE_TIMEOUT)" \
&& "$(SMOKE_TESTS_DIR)/wait-for-it.sh" localhost:8003 --timeout="$(COMPOSE_TIMEOUT)" \
&& "$(SMOKE_TESTS_DIR)/wait-for-it.sh" localhost:8004 --timeout="$(COMPOSE_TIMEOUT)" \

Since ports 8001, 8002, 8003 and 8004 are not exposed on the host we will need another strategy to make sure that the companion containers are up and running.

Maybe using wait-for-it.sh on port 8000 and waiting 15 seconds is enough. You could try to remove lines 75 to 78 and see what happens.

@arukiidou
Copy link
Contributor Author

@arukiidou smoke tests are failing because we are using wait-for-it.sh on port 8001, 8002, 8003 and 8004 in order to make sure that all companion containers are up and running before executing the tests suite.

kroki/Makefile

Lines 74 to 78 in eabc978

&& "$(SMOKE_TESTS_DIR)/wait-for-it.sh" localhost:8000 --timeout="$(COMPOSE_TIMEOUT)" \
&& "$(SMOKE_TESTS_DIR)/wait-for-it.sh" localhost:8001 --timeout="$(COMPOSE_TIMEOUT)" \
&& "$(SMOKE_TESTS_DIR)/wait-for-it.sh" localhost:8002 --timeout="$(COMPOSE_TIMEOUT)" \
&& "$(SMOKE_TESTS_DIR)/wait-for-it.sh" localhost:8003 --timeout="$(COMPOSE_TIMEOUT)" \
&& "$(SMOKE_TESTS_DIR)/wait-for-it.sh" localhost:8004 --timeout="$(COMPOSE_TIMEOUT)" \

Since ports 8001, 8002, 8003 and 8004 are not exposed on the host we will need another strategy to make sure that the companion containers are up and running.

Maybe using wait-for-it.sh on port 8000 and waiting 15 seconds is enough. You could try to remove lines 75 to 78 and see what happens.

Success.

You have two choices. Which is better?

  • close this pr. because the smoke test are legitimate reasons to use ports, not expose.
  • Instead, test it using
>docker compose ps | grep -e blockdiag | grep -e running  | grep -e  8001/tcp
blockdiag           "gunicorn --preload …"   blockdiag           running             8001/tcp
>WAITFORIT_result=$?
>echo $WAITFORIT_result
0

@ggrossetie
Copy link
Member

Let's keep it that way, we can always revisit it if we need/want something more sophisticated.
Thanks!

@ggrossetie ggrossetie changed the title Chore/fix smoke tests docker compose chore(smoketests) don't expose companion container ports Mar 11, 2022
@ggrossetie ggrossetie merged commit 9830ec9 into yuzutech:main Mar 11, 2022
@arukiidou arukiidou deleted the chore/fix-smoke-tests-docker-compose branch July 18, 2022 06:13
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