-
Notifications
You must be signed in to change notification settings - Fork 143
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
Adds subdomain handling #11001 #11002
Conversation
I'm looking forward to testing this. One question so I understand: is it mandatory now to serve stuff from an arches application under a subdomain? Or, if the arches application like arches-for-science doesn't use standalone plugins (whatever we call them), will things still work (i.e. disco can still access the arches for science URLs as they are, without manipulating hosts.py, and webpack still builds)? |
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.
Some very minor feedback from a code read -- will give a functional test once I get the instructions down (might need to meet with you to make sure I'm doing things right)!
Nope not mandatory 😄 -- should be an either or thing && the documentation has ( perhaps prematurely ) been updated to reflect that https://github.com/archesproject/arches-docs/pull/425/files Quite possibly it could even be a both thing, where the same resource is served at both a subdomain and a route -- though I haven't tested it 🤔 |
…nto 11001-cbyrd-subdomains-for-arches-apps
reverting to draft until #11009 is merged 👍 |
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.
+1 tested. Left some minor suggestions.
|
||
host_patterns = patterns( | ||
"", | ||
host(re.sub(r"_", r"-", r"{{ project_name }}"), "{{ project_name }}.urls", name="{{ project_name }}"), |
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.
Think we can remove r
prefix from project_name
.
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.
yeah it looks unnecessary but it's how it's outlined in the docs
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.
no problem to leave this as is, but I'm not seeing re.sub()
in the docs?
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.
ah yeah this is because underscores and domains do not play nicely together, and project_name
can contain underscores.
Adding a domain with an underscore results in:
Invalid HTTP_HOST header: 'foo_bar.localhost:8000'. The domain name provided is not valid according to RFC 1034/1035.
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.
Yeah yeah, I just mean re.sub() returns a not-raw string anyway, so we're already providing a not-raw string -- in that case, no need to pass raw string args to re.sub() such as "arches"
, which isn't a regex. No matter.
|
||
host_patterns = patterns( | ||
"", | ||
host(re.sub(r"_", r"-", r"arches"), "arches.urls", name="arches"), |
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.
same
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.
☝️
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.
Thanks for the updates!
Types of changes
Description of Change
Adds the ability to have subdomains with Arches applications.
To test:
hosts.py
file in the the project to include application's urls. eg:my_arches_app.my_project.domain
These changes remove the concept of a standalone plugin. Instead there is a new template that can extended in projects:
base-root.htm
. It can be extended like so:with a corresponding .js file, eg:
and vue file:
we are now able to load Vue applications without the Arches chrome, and with optional custom theming via the PrimeVue api.
Issues Solved
Closes #11001 #10998 #10997
Checklist
Accessibility Checklist
Developer Guide
Further comments
Documentation: archesproject/arches-docs#425
@robgaston @jacobtylerwalls : This brings up a few interesting discussion points.
1. Do we really need the new template. This has been replaced withstandalone-component-base.htm
? There seems to be some cruft inbase.htm
, and it pulls injavascript.htm
which heavy, but still no UI chrome. If we decided to usebase.htm
instead, we can undo the business witharches_urls
being moved into its own template.base-root.htm,
whichbase.htm
inherits fromstandalone-component-base.htm
orbase.htm
, there's still the issue of extending the template at the project level. This is fine and good and supported for now, but if we're trying to trend to not have a frontend bundler eat templates then we need to abandon Django template interpolation.3. There is significant overlap now between plugins and Arches applications. Not in 7.6 of course, but should we consider eventually dropping support for plugins?