Skip to content

Improve PythonFdWrapper class#1846

Merged
BareosBot merged 3 commits intobareos:masterfrom
arogge:dev/arogge/master/improve-fd-wrapper
Jul 10, 2024
Merged

Improve PythonFdWrapper class#1846
BareosBot merged 3 commits intobareos:masterfrom
arogge:dev/arogge/master/improve-fd-wrapper

Conversation

@arogge
Copy link
Member

@arogge arogge commented Jun 6, 2024

This PR tries to improve the PythonFdWrapper.
With these changes applied

  • exceptions will be caught within the python code and made into JobMessages
  • from PythonFdWrapper import * will now only import some of the names
  • pylint-compliance

Please check

  • Short description and the purpose of this PR is present above this paragraph
  • Your name is present in the AUTHORS file (optional)

If you have any questions or problems, please give a comment in the PR.

Helpful documentation and best practices

Checklist for the reviewer of the PR (will be processed by the Bareos team)

Make sure you check/merge the PR using devtools/pr-tool to have some simple automated checks run and a proper changelog record added.

General
  • Is the PR title usable as CHANGELOG entry?
  • Purpose of the PR is understood
  • Commit descriptions are understandable and well formatted
    Required backport PRs have been created
Source code quality
  • Source code changes are understandable
  • Variable and function names are meaningful
  • Code comments are correct (logically and spelling)
  • Required documentation changes are present and part of the PR

@arogge arogge marked this pull request as draft June 6, 2024 11:07
@arogge
Copy link
Member Author

arogge commented Jun 18, 2024

The new exception handling is not wanted/required, but all the other improvements should be merged.

@arogge arogge self-assigned this Jun 18, 2024
@arogge arogge added this to the 24.0.0 milestone Jun 25, 2024
@arogge arogge force-pushed the dev/arogge/master/improve-fd-wrapper branch from 8a3d68e to 1abd35d Compare July 1, 2024 14:47
@arogge arogge marked this pull request as ready for review July 1, 2024 14:47
@arogge
Copy link
Member Author

arogge commented Jul 1, 2024

I made it so that all functions bubble up their exceptions, even if they didn't do that previously. i.e. we had exception handling in load_bareos_plugin() previously, but now this also bubbles up to python-fd.

Copy link
Contributor

@sebsura sebsura left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@arogge arogge force-pushed the dev/arogge/master/improve-fd-wrapper branch from 5fd3a33 to c0de743 Compare July 9, 2024 08:27
arogge and others added 3 commits July 10, 2024 15:52
The wrappers won't catch any exceptions, but will let python-fd's
error handler take care of them. Previously this was a bit unclear as
load_bareos_plugin() did handle exceptions, but the other wrapper
functions did not.

We now declare what names will be exported to consuming plugins. This
ensures we can change the implementation as long as we don't break the
API. This also ensures that we don't leak implementation details into
the plugins using the wrapper.

improve compliance with pylint. Some warnings can only be ignored.
Especially naming issues in the API are not fixable in a
backwards-compatible manner.
We currently have "bugfix" and "bug" as labels in the Bareos project.
The "bugfix" label was added for PRs that fix a bug. However, since we
started using issues we also have the "bug" label for issues that
descibe a bug. We should probably use "bug" for everything, but for
that pr-tool needs to accept "bug" as an alias for "bugfix" when
generating changelog records.
@BareosBot BareosBot force-pushed the dev/arogge/master/improve-fd-wrapper branch from a1b37bf to 0851a7d Compare July 10, 2024 15:52
@BareosBot BareosBot merged commit 3bb317e into bareos:master Jul 10, 2024
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.

3 participants