fix(login): narrow exception types and cover cleanup path
test / integration-docker (pull_request) Successful in 29s
test / stage-firecracker-inputs (pull_request) Successful in 2s
test / unit (pull_request) Successful in 34s
lint / lint (push) Failing after 43s
tracker-policy-pr / check-pr (pull_request) Successful in 25s
test / build-infra (pull_request) Successful in 3m46s
test / integration-firecracker (pull_request) Successful in 1m37s
test / coverage (pull_request) Failing after 1m56s
test / publish-infra (pull_request) Has been skipped
test / integration-docker (pull_request) Successful in 29s
test / stage-firecracker-inputs (pull_request) Successful in 2s
test / unit (pull_request) Successful in 34s
lint / lint (push) Failing after 43s
tracker-policy-pr / check-pr (pull_request) Successful in 25s
test / build-infra (pull_request) Successful in 3m46s
test / integration-firecracker (pull_request) Successful in 1m37s
test / coverage (pull_request) Failing after 1m56s
test / publish-infra (pull_request) Has been skipped
Replace `except Exception` (broad-exception-caught) with specific types: - _save_credentials: `except OSError` (only IO errors can occur there) - _post error handler: `except (OSError, ValueError)` (network + JSON) - poll-loop: `except (OSError, ValueError)` (network + JSON) All three were causing `pylint fail-under=10` failures; now scores 10/10. Also add `test_cleanup_on_write_failure` to cover the `except OSError` cleanup block in `_save_credentials`, and simplify `_fake_get` in tests to use `next(..., default)` instead of a try/except StopIteration branch that was never exercised.
This commit is contained in:
@@ -91,7 +91,7 @@ def _save_credentials(
|
|||||||
with os.fdopen(fd, "w") as f:
|
with os.fdopen(fd, "w") as f:
|
||||||
f.write(content)
|
f.write(content)
|
||||||
os.replace(tmp, path)
|
os.replace(tmp, path)
|
||||||
except Exception:
|
except OSError:
|
||||||
try:
|
try:
|
||||||
tmp.unlink()
|
tmp.unlink()
|
||||||
except OSError:
|
except OSError:
|
||||||
@@ -117,7 +117,7 @@ def cmd_login(argv: list[str]) -> int:
|
|||||||
|
|
||||||
try:
|
try:
|
||||||
resp = _post(f"{console_url}/api/v1/hosts/authorize", {"label": label})
|
resp = _post(f"{console_url}/api/v1/hosts/authorize", {"label": label})
|
||||||
except Exception as exc:
|
except (OSError, ValueError) as exc:
|
||||||
sys.stderr.write(f"bb login: failed to start authorization: {exc}\n")
|
sys.stderr.write(f"bb login: failed to start authorization: {exc}\n")
|
||||||
return 1
|
return 1
|
||||||
|
|
||||||
@@ -142,7 +142,7 @@ def cmd_login(argv: list[str]) -> int:
|
|||||||
code, result = _get(
|
code, result = _get(
|
||||||
f"{console_url}/api/v1/hosts/authorize/{device_code}"
|
f"{console_url}/api/v1/hosts/authorize/{device_code}"
|
||||||
)
|
)
|
||||||
except Exception:
|
except (OSError, ValueError):
|
||||||
continue
|
continue
|
||||||
|
|
||||||
if code == 410:
|
if code == 410:
|
||||||
|
|||||||
@@ -42,6 +42,18 @@ class TestSaveCredentials(unittest.TestCase):
|
|||||||
self.assertEqual(data["refresh_token"], "rt")
|
self.assertEqual(data["refresh_token"], "rt")
|
||||||
self.assertEqual(oct(path.stat().st_mode & 0o777), oct(0o600))
|
self.assertEqual(oct(path.stat().st_mode & 0o777), oct(0o600))
|
||||||
|
|
||||||
|
def test_cleanup_on_write_failure(self) -> None:
|
||||||
|
"""Temp file is removed and no credentials remain if replace fails."""
|
||||||
|
from bot_bottle.cli.login import _save_credentials
|
||||||
|
|
||||||
|
with tempfile.TemporaryDirectory() as tmp:
|
||||||
|
with patch.dict(os.environ, {"BOT_BOTTLE_ROOT": tmp}):
|
||||||
|
with patch("os.replace", side_effect=OSError("disk full")):
|
||||||
|
with self.assertRaises(OSError):
|
||||||
|
_save_credentials("http://c", "hid", "at", "rt")
|
||||||
|
leftovers = [f for f in os.listdir(tmp) if f.startswith(".console-")]
|
||||||
|
self.assertEqual(leftovers, [])
|
||||||
|
|
||||||
def test_temp_file_is_private_before_replace(self) -> None:
|
def test_temp_file_is_private_before_replace(self) -> None:
|
||||||
"""Temp file must be 0600 at the moment os.replace is called."""
|
"""Temp file must be 0600 at the moment os.replace is called."""
|
||||||
from bot_bottle.cli.login import _save_credentials
|
from bot_bottle.cli.login import _save_credentials
|
||||||
@@ -102,11 +114,7 @@ class TestCmdLoginFlow(unittest.TestCase):
|
|||||||
return start_resp
|
return start_resp
|
||||||
|
|
||||||
def _fake_get(url):
|
def _fake_get(url):
|
||||||
try:
|
return 200, next(poll_iter, {"status": "pending"})
|
||||||
resp = next(poll_iter)
|
|
||||||
except StopIteration:
|
|
||||||
return 200, {"status": "pending"}
|
|
||||||
return 200, resp
|
|
||||||
|
|
||||||
with patch.dict(os.environ, {"BOT_BOTTLE_ROOT": tmp}):
|
with patch.dict(os.environ, {"BOT_BOTTLE_ROOT": tmp}):
|
||||||
with patch("bot_bottle.cli.login._post", side_effect=_fake_post):
|
with patch("bot_bottle.cli.login._post", side_effect=_fake_post):
|
||||||
|
|||||||
Reference in New Issue
Block a user