Merge pull request #1062 from GitGuardian/severine/dont-block-push-on-server-503

feat: do not block pre-receive when server isn't responding (503)

Author: sevbch

changelog.d/20250303_151712_severine.bonnechere_dont_block_push_on_server_503.md

@@ -0,0 +1,3 @@
+### Changed
+
+- Pre-receive hook isn't blocking anymore when GitGuardian server is temporarily unavailable (return 5xx status code).

ggshield/cmd/secret/scan/prereceive.py

@@ -17,7 +17,7 @@
 from ggshield.core.cache import ReadOnlyCache
 from ggshield.core.client import create_client_from_config
 from ggshield.core.config import Config
-from ggshield.core.errors import handle_exception
+from ggshield.core.errors import ExitCode, handle_exception
 from ggshield.core.git_hooks.prereceive import (
     get_breakglass_option,
     get_prereceive_timeout,
@@ -137,5 +137,8 @@ def prereceive_cmd(
         ui.display_error("\nPre-receive hook took too long")
         process.kill()
         return 0
+    if process.exitcode == ExitCode.GITGUARDIAN_SERVER_UNAVAILABLE:
+        ui.display_error("\nGitGuardian server is not responding. Skipping checks.")
+        return 0
 
     return process.exitcode

ggshield/core/client.py

@@ -8,7 +8,12 @@
 
 from .config import Config
 from .constants import DEFAULT_INSTANCE_URL
-from .errors import APIKeyCheckError, UnexpectedError, UnknownInstanceError
+from .errors import (
+    APIKeyCheckError,
+    ServiceUnavailableError,
+    UnexpectedError,
+    UnknownInstanceError,
+)
 from .ui.client_callbacks import ClientCallbacks
 
 
@@ -103,9 +108,13 @@ def check_client_api_key(client: GGClient) -> None:
         raise APIKeyCheckError(client.base_uri, "Invalid API key.")
     elif response.status_code == 404:
         raise UnexpectedError(
-            "The server returned a 404 error. Check your instance URL" " settings."
+            "The server returned a 404 error. Check your instance URL settings.",
+        )
+    elif response.status_code is not None and 500 <= response.status_code < 600:
+        raise ServiceUnavailableError(
+            message=f"GitGuardian server is not responding.\nDetails: {response.detail}",
         )
     else:
         raise UnexpectedError(
-            f"Server is not responding as expected.\nDetails: {response.detail}"
+            f"GitGuardian server is not responding as expected.\nDetails: {response.detail}"
         )

ggshield/core/errors.py

@@ -33,6 +33,8 @@ class ExitCode(IntEnum):
     USAGE_ERROR = 2
     # auth subcommand failed
     AUTHENTICATION_ERROR = 3
+    # GitGuardian server is not responding
+    GITGUARDIAN_SERVER_UNAVAILABLE = 4
 
     # Add new exit codes here.
     # If you add a new exit code, make sure you also add it to the documentation.
@@ -133,6 +135,15 @@ def __init__(self, instance: str, message: str):
         super().__init__(instance, message)
 
 
+class ServiceUnavailableError(_ExitError):
+    """
+    Raised when the server is unavailable
+    """
+
+    def __init__(self, message: str):
+        super().__init__(ExitCode.GITGUARDIAN_SERVER_UNAVAILABLE, message)
+
+
 def format_validation_error(exc: ValidationError) -> str:
     """
     Take a Marshmallow ValidationError and turn it into a more user-friendly message

tests/unit/cmd/scan/test_prereceive.py

@@ -2,6 +2,7 @@
 
 import pytest
 from click.testing import CliRunner
+from pygitguardian.models import Detail
 
 from ggshield.__main__ import cli
 from ggshield.core.config.user_config import SecretConfig
@@ -363,3 +364,27 @@ def test_stdin_input_deletion(
         )
         assert_invoke_ok(result)
         assert "Deletion event or nothing to scan.\n" in result.output
+
+    @patch(
+        "pygitguardian.client.GGClient.read_metadata",
+        return_value=Detail("Service is unavailable", 503),
+    )
+    def test_server_unavailable(self, _: Mock, tmp_path, cli_fs_runner: CliRunner):
+        """
+        GIVEN a repo on which the command is ran
+        WHEN the server is not responding (503)
+        THEN it should return 0
+        AND display an error message
+        """
+        # setting up repo to run the command
+        repo = create_pre_receive_repo(tmp_path)
+        old_sha = repo.get_top_sha()
+        shas = [repo.create_commit() for _ in range(3)]
+        with cd(repo.path):
+            result = cli_fs_runner.invoke(
+                cli,
+                ["-v", "secret", "scan", "pre-receive"],
+                input=f"{old_sha} {shas[-1]} origin/main\n",
+            )
+        assert_invoke_ok(result)
+        assert "GitGuardian server is not responding" in result.output

tests/unit/cmd/scan/test_repo.py

@@ -1,3 +1,7 @@
+from unittest.mock import Mock, patch
+
+from pygitguardian.models import Detail
+
 from ggshield.__main__ import cli
 from ggshield.core.errors import ExitCode
 from tests.unit.conftest import assert_invoke_exited_with
@@ -35,3 +39,20 @@ def test_invalid_scan_repo_url(self, cli_fs_runner):
             "Error: trial.gitguardian.com/gitguardian/ggshield is"
             " neither a valid path nor a git URL" in result.output
         )
+
+    @patch(
+        "pygitguardian.client.GGClient.read_metadata",
+        return_value=Detail("Service is unavailable", 503),
+    )
+    def test_server_unavailable(self, _: Mock, cli_fs_runner):
+        """
+        GIVEN a server that is unavailable
+        WHEN scan repo is called
+        THEN it should return 0
+        """
+        result = cli_fs_runner.invoke(
+            cli,
+            ["secret", "scan", "repo", "https://github.com/gitguardian/ggshield.git"],
+        )
+        assert_invoke_exited_with(result, ExitCode.GITGUARDIAN_SERVER_UNAVAILABLE)
+        assert "GitGuardian server is not responding" in result.output

tests/unit/core/test_client.py

@@ -10,13 +10,18 @@
 
 from ggshield.core.client import check_client_api_key, create_client_from_config
 from ggshield.core.config import Config
-from ggshield.core.errors import APIKeyCheckError, UnexpectedError, UnknownInstanceError
+from ggshield.core.errors import (
+    APIKeyCheckError,
+    ServiceUnavailableError,
+    UnexpectedError,
+    UnknownInstanceError,
+)
 
 
 @pytest.mark.parametrize(
     ("response", "error_class"),
     (
-        (Detail("Guru Meditation", 500), UnexpectedError),
+        (Detail("Guru Meditation", 500), ServiceUnavailableError),
         (Detail("Nobody here", 404), UnexpectedError),
         (Detail("Unauthorized", 401), APIKeyCheckError),
     ),
@@ -42,6 +47,7 @@ def test_check_client_api_key_network_error():
     """
     client_mock = Mock()
     client_mock.health_check = Mock(side_effect=requests.exceptions.ConnectionError)
+    client_mock.read_metadata = Mock(return_value=Detail("Not found", 404))
     with pytest.raises(UnexpectedError):
         check_client_api_key(client_mock)