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

Replace curl by requests #290

Merged
merged 1 commit into from Aug 21, 2023
Merged

Replace curl by requests #290

merged 1 commit into from Aug 21, 2023

Conversation

grisu48
Copy link
Contributor

@grisu48 grisu48 commented Aug 17, 2023

Replace the curl utility by the python requests library to avoid another
host-dependency.

@clanig
Copy link

clanig commented Aug 17, 2023

Thanks a lot for taking care of this!
If the option --retry-connrefused is removed, I assume it will exit with 7 if the service is not available and not retry any more.

Since I guess, waiting for the service to become ready is the reason for the retry attempts, it would be similar to run without any retries.

I would suggest to wrapping the command itself in an own custom retry logic, not using any of the retry functionality provided by curl itself.

Edit: I like the replacement with requests even better.

@grisu48 grisu48 changed the title Remove --retry-connrefused Replace curl by requests Aug 17, 2023
@clanig
Copy link

clanig commented Aug 17, 2023

LGTM.

tests/test_nginx.py Outdated Show resolved Hide resolved
@grisu48 grisu48 force-pushed the res7 branch 3 times, most recently from 23f4c96 to 344f38f Compare August 17, 2023 14:53
)
def check_reply():
resp = requests.get(f"http://localhost:{host_port}/", timeout=30)
Copy link
Member

Choose a reason for hiding this comment

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

this can be

+        assert "Welcome to nginx" in host.check_output(
+            f"curl -sf --retry 5 http://localhost:{host_port}/"

instead, which avoids the requests dependency (don#t really care either way)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No preference either, @dcermak you pick.

Copy link
Member

Choose a reason for hiding this comment

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

tests/test_ruby.py needs to be updated then with the same change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's first see what @dcermak picks.

@dirkmueller
Copy link
Member

full tested patch

--- a/tests/test_nginx.py
+++ b/tests/test_nginx.py
@@ -1,5 +1,6 @@
 """This module contains the tests for the nginx container, the image with nginx pre-installed.
 """
+from tenacity import retry, stop_after_delay
 from bci_tester.data import NGINX_CONTAINER
 
 
@@ -10,6 +11,9 @@ def test_nginx_welcome_page(auto_container, host):
     """test that the default welcome page is served by the container."""
     host_port = auto_container.forwarded_ports[0].host_port
 
-    assert "Welcome to nginx" in host.check_output(
-        f"curl -sf --retry 5 --retry-connrefused http://localhost:{host_port}/"
-    )
+    @retry(stop=stop_after_delay(10))
+    def check_nginx_response():
+        assert "Welcome to nginx" in host.check_output(
+            f"curl -sf --retry 5 http://localhost:{host_port}/"
+        )
+    check_nginx_response()
diff --git a/tox.ini b/tox.ini
index 290d137..880cf2e 100644
--- a/tox.ini
+++ b/tox.ini
@@ -10,6 +10,7 @@ deps =
     pytest-xdist ; python_version >= "3.6"
     dataclasses ; python_version < "3.7"
     pytest-rerunfailures
+    tenacity
     typing_extensions
     git+https://github.com/dcermak/pytest_container
     doc: Sphinx

@grisu48 grisu48 force-pushed the res7 branch 2 times, most recently from 6036a85 to 9b51f08 Compare August 17, 2023 15:03
tests/test_nginx.py Outdated Show resolved Hide resolved
@grisu48 grisu48 force-pushed the res7 branch 2 times, most recently from 3c6504d to 4e330fc Compare August 21, 2023 07:48
tests/test_nginx.py Outdated Show resolved Hide resolved
Replace the curl utility by the python requests library to avoid another
host-dependency.
@dcermak dcermak enabled auto-merge August 21, 2023 09:02
@dcermak dcermak merged commit 73bc2cb into SUSE:main Aug 21, 2023
78 checks passed
@grisu48 grisu48 deleted the res7 branch August 21, 2023 09:51
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.

None yet

4 participants