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

Add settings toggle for local frontend #899

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

hmpf
Copy link
Contributor

@hmpf hmpf commented Oct 9, 2024

May be replaced with something else that depends on #904

Adds a setting to turn the new frontend on and the old frontend off. It is not possible to run both at the same time due to some hackery the old frontend needs for OAuth2.

@hmpf hmpf self-assigned this Oct 9, 2024
@hmpf hmpf requested review from a team October 9, 2024 06:49
@hmpf hmpf added enhancement New feature or request frontend Has/needs companion issue in frontend priority: high dependencies Run `tox -r` before testing locally, dependencies have changed blocked Another thing/issue has to be resolved before tackling this auth Touches the authentication/authorization subsystem HTMx Views, urls, templates... labels Oct 9, 2024
@elfjes
Copy link
Collaborator

elfjes commented Oct 9, 2024

Hmm I don't think I like this. Without having thorouhly examined the implications of this PR it feels like it makes Argus more opinionated in how it wants to be used/configured. For example, we're not using the apps plugin system, which means that we're currently fully outside the flow of EXTRA_APPS and OVERRIDING_APPS. We have our own way of including argus_htmx. It feels like this PR wants to nudge us to a different method, and I fear we'll lose the flexibility we have now. We'd maybe want to set FRONTEND_APP to None to not have Argus include argus_htmx itself, but then Argus assumes we run the old fontend/api-only and that's also not what we want.

Like I said, I haven't looked at the implications thoroughly, but this is my initial impression.

I'd rather see Argus creating a new settings files that can be pointed to in prod that sets up argus for htmx usage rather than to include everything in site/settings/base.py and site/urls.,py, and then we can happily not do anything with those files :)

pehaps the oauth functionality can become an app in and of itself than can be included/excluded at leisure

@hmpf
Copy link
Contributor Author

hmpf commented Oct 9, 2024

Yeah I realized the oauth stuff can be split out into a different PR, BUT: the current, old-frontend oauth2 stuff is hardcoded to the vendored psa backend because of the loginwrapper. So in order to use ANY oauth2 we need to know that we are NOT using the old frontend, so we need a switch for that. FRONTEND_URL the setting is also used everywhere and with kubernetes it makes it easier to have the setting because django struggles to find what host it publicly runs on otherwise. (If we log with hostname from kubernetes we do not get a hostname but the node/pod-id. Much useful, very quality.)

@hmpf
Copy link
Contributor Author

hmpf commented Oct 9, 2024

I thought this way of pulling in all the settings the frontend needs, while still supporting the old frontend, was quite neat. Every setting that argus_htmx must have is in argus_htmx.appconfig, and every setting can be overruled if necessary/as usual in a localsettings file.

This is still compatible with (OVERRIDING|EXTRA)_APPS, and worst case it is always possible to clone base.py completely. The trick is, again, the site urls.py. But what root urls.py to use is still a setting.

@elfjes
Copy link
Collaborator

elfjes commented Oct 9, 2024

Oh I'm all for factoring out the hardcoded oauth stuff into a separate thing (eg app). My concern is with settings/base and site/urls. I'd much rather see that being more composable rather than behaviour basecd on toggles. That way we can each compose our own settings/urls as we see fit. IMHO Argus should become less opinionated on how it's used/integrated, not more.

I think that requiring cloning base.py should only be done as a last resort, since we want to minimize code duplication.

@hmpf hmpf marked this pull request as draft October 10, 2024 12:54
.. and due to this, move the overriding to the end of base settings
.. this makes it easy to change settings that are not app-specific, like
LOGIN_URL.
.. make sure the examples are valid JSON
Copy link

sonarcloud bot commented Oct 16, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
37.0% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auth Touches the authentication/authorization subsystem blocked Another thing/issue has to be resolved before tackling this dependencies Run `tox -r` before testing locally, dependencies have changed enhancement New feature or request frontend Has/needs companion issue in frontend HTMx Views, urls, templates... priority: high
Projects
Status: 🏗 In progress
Development

Successfully merging this pull request may close these issues.

2 participants