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

Exit synadm early on fatal errors, refactor & fix query() #168

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

JOJ0
Copy link
Owner

@JOJ0 JOJ0 commented Jan 24, 2025

Addresses general error handling issues discussed in: #164

  • What actually is a fatal error where it does not make sense to continue? I picked a few already. There might be more. Please suggest!
  • Is it really a good idea to define log_fatal_exit() directly in this module as a regular function? Should it live inside ApiRequest? Or move it to a "utils-like" place? Will we use it outside this module too?
  • Some refactoring along the way
  • Add type hints (we can discuss if this is the right place and time....)
  • Fix docstring
    • arguments done
    • return value: FIXME, isn't this entirely wrong? resp.json() returns lists or dicts. Or what does json string mean in this context? It's no string in reality....

@JOJ0 JOJ0 marked this pull request as draft January 24, 2025 06:40
@JOJ0 JOJ0 requested a review from JacksonChen666 January 24, 2025 07:41
@JOJ0 JOJ0 marked this pull request as ready for review January 24, 2025 08:34
@JOJ0 JOJ0 changed the title Exit synadm early on fatal errors Exit synadm early on fatal errors, refactor & fix query() Jan 24, 2025
@JacksonChen666
Copy link
Collaborator

  • What actually is a fatal error where it does not make sense to continue? I picked a few already. There might be more. Please suggest!

I guess check for anything that logs errors and exits. Searching for raise SystemExit is a good starting point.

  • Is it really a good idea to define log_fatal_exit() directly in this module as a regular function? Should it live inside ApiRequest? Or move it to a "utils-like" place? Will we use it outside this module too?

Maybe do move it to some utils place (tying into the last point)

  • return value: FIXME, isn't this entirely wrong? resp.json() returns lists or dicts. Or what does json string mean in this context? It's no string in reality....

JSONs can also be strings, numbers, booleans, nulls, and the usually expected lists and dicts. In most (or even all) cases, it's dicts and possibly lists. So it can be kind of anything, but for specific API methods, we might as well just infer from the example output in Synapse Admin API docs, and for the query function, just JSON decoded I guess (noting that other types are possible too).

Copy link
Collaborator

@JacksonChen666 JacksonChen666 left a comment

Choose a reason for hiding this comment

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

Looked at it, made some commits. Hope to get some feedback on some of my commits, if any.

synadm/api.py Outdated Show resolved Hide resolved
synadm/api.py Show resolved Hide resolved
synadm/api.py Outdated Show resolved Hide resolved
synadm/api.py Outdated Show resolved Hide resolved
synadm/api.py Show resolved Hide resolved
JOJ0 added 2 commits January 31, 2025 18:28
and don't overwrite built-in ConnectionError with import statement.
@JOJ0
Copy link
Owner Author

JOJ0 commented Jan 31, 2025

Good to go? @JacksonChen666

@JOJ0
Copy link
Owner Author

JOJ0 commented Jan 31, 2025

Good to go? @JacksonChen666

Ah moving to utils place......but actually: We don't have a place for it. I'd rather not invent one now. We do that if we ever need it elsewhere. Agreed?

(we only have _helper.py and _common.py in package cli which is not imported here, so...)

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.

room power-levels should fail gracefully when Synapse not reachable
2 participants