-
Notifications
You must be signed in to change notification settings - Fork 30
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
Urls import handling #79
Conversation
As discussed: strategy of failing on import is 👍 but would be nice to have more friendly error messages. |
With that last commit, you now see this for failed apps (weberror) Show error
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR has been in the build for a while now. In general, 👍 for more feedback to the users when something has gone wrong. One minor comment that can likely be addressed and merged with no further testing.
else: | ||
regex = '^(?i)%s/' % label | ||
urlpatterns.append(url(regex, include(urlmodule))) | ||
logger.debug('Module not found: %s' % urlmodule) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be higher than "debug"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. This just means that an app doesn't have a urls.py
. E.g. corsheaders
. It's not a failure since we don't expect all apps to have it.
I guess thee is an expectation that omero web apps will have a urls.py
, but we have no concrete way of telling which app is an omero-web app.
Perhaps if urlmodule.startswith("ome")
we could log at info, or even error? But it's still possible this is not really an error. You're not likely to accidentally forget to add a urls.py
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least not and expect it to work 😄 Thanks for the explanation!
This change means that failure to import
module.urls.py
for OMERO.web apps results in a failure of OMERO.web to run. Failure to import themodule
itself has always resulted in a failure of OMERO.web to run, but before this PR, import ofmodule.urls.py
fails silently.This has meant that an app could be "installed" and OMERO.web appear to run OK, but all of the app's URLS are 404 or that
reverse(url_name)
fails.This PR implements the behaviour that I manually applied each time I was working to port an OMERO.web app to python3 where I needed to see import failures at start-time, and not silently result in 404s.
Hopefully this will also help fixing failure of
reverse("mapr_config")
etc in integration tests at ome/omero-mapr#54 by causing a failure at an earlier point.To test, run OMERO.web locally, install an app (e.g figure, iviewer etc) then add an error that causes an
ImportError
, either in theurls.py
orviews.py
and check that OMERO.web fails withImportError
. Could also use a pre-python3 version of figure or iviewer etc.