Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Allow dependency errors to pass through #13113

Merged

Conversation

Vetchu
Copy link
Contributor

@Vetchu Vetchu commented Jun 25, 2022

Fixes #13066.

I think this was the author meant in the original issue: I'm not exactly sure how to grab the jwt one though, please advise.

Also mypy threw an error on my side which I do not understand: is there something missing? When removing try/catch the potentially exception-raising part is not visible well.

Signed-off-by: Jacek Kusnierz [email protected]

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Pull request includes a sign off
  • Code style is correct
    (run the linters)

Vetchu added 2 commits June 25, 2022 09:38
Signed-off-by: Jacek Kusnierz <[email protected]>
@Vetchu Vetchu marked this pull request as ready for review June 26, 2022 14:30
@Vetchu Vetchu requested a review from a team as a code owner June 26, 2022 14:30
@babolivier babolivier self-assigned this Jun 27, 2022
Copy link
Contributor

@babolivier babolivier left a comment

Choose a reason for hiding this comment

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

Thanks for contributing to Synapse!
As far as I can tell there's one bit that's missing from your PR:

try:
import jwt
jwt # To stop unused lint.
except ImportError:
raise ConfigError(MISSING_JWT)

Which still raises a ConfigError instead of a DependencyException. I believe this one can be fixed by replacing this block with:

check_requirements("jwt")

(and removing the then unused MISSING_AUTHLIB constant)

changelog.d/13113.misc Outdated Show resolved Hide resolved
@DMRobertson
Copy link
Contributor

I think this was the author meant in the original issue: I'm not exactly sure how to grab the jwt one though, please advise.

Thanks for this: these changes are exactly what I had in mind.

@Vetchu Vetchu requested a review from babolivier June 28, 2022 08:58
@Vetchu
Copy link
Contributor Author

Vetchu commented Jun 29, 2022

I'm not sure how to perform check_requirements function on the authlib (which replaced jwt): can you point me to a documentation about how is it checked?

@DMRobertson
Copy link
Contributor

I'm not sure how to perform check_requirements function on the authlib (which replaced jwt): can you point me to a documentation about how is it checked?

#13113 (review) explains what needs to be done.

In a little more detail:

  • the argument to check_requirements is the name of a package "extra" feature
  • the relevant extra here is jwt, which comes from here
    jwt = ["authlib"]

Signed-off-by: Jacek Kusnierz <[email protected]>
Copy link
Contributor

@babolivier babolivier 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 apart from this, sorry for not explaining my previous comment enough (and thanks David for jumping in)!

Install by running:
pip install synapse[jwt]
"""
from ..util.check_dependencies import check_requirements
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
from ..util.check_dependencies import check_requirements
from synapse.util.check_dependencies import check_requirements

When modules aren't in the current directory we usually prefer if the import path is absolute :)

This will need linting (because github's suggestions apparently don't allow me to move the import statement a line above 🙁 )

Copy link
Contributor

@babolivier babolivier 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 now, thanks for your contribution! 🙂

@babolivier babolivier merged commit 50f0e40 into matrix-org:develop Jun 30, 2022
@Vetchu Vetchu deleted the chore/raise-dependency-not-config-errors branch July 11, 2022 13:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't raise ConfigErrors when dependencies are missing
3 participants