Skip to content

fix(backups): pass both stdout and stderr to _extract_error_message in _list_timelines#1666

Open
SAY-5 wants to merge 1 commit into
canonical:mainfrom
SAY-5:fix/issue-1482-list-timelines-extract-error-args
Open

fix(backups): pass both stdout and stderr to _extract_error_message in _list_timelines#1666
SAY-5 wants to merge 1 commit into
canonical:mainfrom
SAY-5:fix/issue-1482-list-timelines-extract-error-args

Conversation

@SAY-5
Copy link
Copy Markdown
Contributor

@SAY-5 SAY-5 commented Apr 30, 2026

Summary

Closes #1482.

_extract_error_message is declared with two positional arguments (stdout, stderr):

@staticmethod
def _extract_error_message(stdout: str, stderr: str) -> str:

Every other call site in src/backups.py (L214, L457, L531, L747, L811, L1028, L1195) supplies both arguments. The _list_timelines call at L570 was the lone exception:

extracted_error = self._extract_error_message(stderr)

so whenever pgbackrest repo-ls returned a non-zero exit code, the error path raised

TypeError: _extract_error_message() missing 1 required positional argument: 'stderr'

instead of the intended ListBackupsError, masking the underlying backup failure.

Fix

Pass the local output variable (the stdout captured from _execute_command) as the first argument, matching the surrounding convention. One-line change.

Test plan

  • python -c 'import ast; ast.parse(open("src/backups.py").read())', syntax OK
  • grep _extract_error_message src/backups.py, all 8 call sites now pass two arguments
  • CI: integration tests that hit a failing pgbackrest repo-ls (e.g. revoked credentials) should now surface the real error inside ListBackupsError instead of the TypeError

…n _list_timelines

`_extract_error_message` is declared as

    @staticmethod
    def _extract_error_message(stdout: str, stderr: str) -> str:

Every other call site in src/backups.py (lines 214, 457, 531, 747,
811, 1028, 1195) supplies both arguments. The `_list_timelines`
call at line 570 was the lone exception:

    extracted_error = self._extract_error_message(stderr)

so whenever `pgbackrest repo-ls` returned a non-zero exit code, the
error path raised

    TypeError: _extract_error_message() missing 1 required positional
    argument: 'stderr'

instead of the intended ListBackupsError, masking the underlying
backup failure.

Pass the local `output` (stdout) variable as the first argument to
match the surrounding convention.

Closes canonical#1482.

Signed-off-by: SAY-5 <say.apm35@gmail.com>
@SAY-5 SAY-5 requested a review from a team as a code owner April 30, 2026 19:23
@SAY-5 SAY-5 requested review from carlcsaposs-canonical, dragomirp, juju-charm-bot, marceloneppel and taurus-forever and removed request for a team April 30, 2026 19:23
@dragomirp dragomirp added the bug Something isn't working as expected label Apr 30, 2026
Copy link
Copy Markdown
Contributor

@taurus-forever taurus-forever left a comment

Choose a reason for hiding this comment

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

Thank you!

@marceloneppel
Copy link
Copy Markdown
Member

Thanks, @SAY-5!

The CI blocked the merge due to an issue regarding your commit signature. Could you check it?

@SAY-5
Copy link
Copy Markdown
Contributor Author

SAY-5 commented May 5, 2026

Thanks @marceloneppel! Will re-push with a signed commit shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working as expected

Projects

None yet

Development

Successfully merging this pull request may close these issues.

_list_timelines calls _extract_error_message with wrong number of arguments

4 participants