Argus icon indicating copy to clipboard operation
Argus copied to clipboard

Add settings toggle for local frontend

Open hmpf opened this issue 1 year ago • 5 comments

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 avatar Oct 09 '24 06:10 hmpf

Previously, having a setting in argus.site.settings.base that toggled between old and new frontend via lots of code and a new setting:

elfjes: 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: 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: 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: 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 avatar Oct 18 '24 11:10 hmpf

Codecov Report

Attention: Patch coverage is 0% with 13 lines in your changes missing coverage. Please review.

Project coverage is 83.36%. Comparing base (ce09d7e) to head (ca607aa). Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/argus/htmx/htmx_urls.py 0.00% 5 Missing :warning:
src/argus/htmx/settings.py 0.00% 5 Missing :warning:
src/argus/htmx/urls.py 0.00% 3 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #899      +/-   ##
==========================================
- Coverage   83.63%   83.36%   -0.27%     
==========================================
  Files          95       98       +3     
  Lines        4137     4149      +12     
==========================================
- Hits         3460     3459       -1     
- Misses        677      690      +13     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Oct 21 '24 07:10 codecov-commenter

Question, if we'd want to use OVERRIDING_APPS / EXTRA_APPS. we'd want to do this without environment variables but add the relevant structs directly in our settings.py. Right now the only way I can see that we could do that is by calling update_settings ourselves, right?

update_settings needs to be run in the final, outermost settings-file yes. Remember globals(). No env-vars needed.

update_settings now takes the boolean flag override (default: False).

It would be nice if we could do something like this:

geant_settings.py

from argus_htmx import app_settings

update_settings(app_settings)

That way we can have the magic, while still being in control of it :)

Just copy the stuff from argus.htmx.settings into the geant settings, then override what you need. If you have your own geant app_settings you could run update_settings twice.

Either make an instance of type ListAppSetting or convert a list of dicts of settings via ListAppSetting(list_of_dicts).

Is more docs needed?

hmpf avatar Oct 21 '24 10:10 hmpf

Question, if we'd want to use OVERRIDING_APPS / EXTRA_APPS. we'd want to do this without environment variables but add the relevant structs directly in our settings.py. Right now the only way I can see that we could do that is by calling update_settings ourselves, right?

update_settings needs to be run in the final, outermost settings-file yes. Remember globals(). No env-vars needed.

update_settings now takes the boolean flag override (default: False).

It would be nice if we could do something like this: geant_settings.py

from argus_htmx import app_settings

update_settings(app_settings)

That way we can have the magic, while still being in control of it :)

Just copy the stuff from argus.htmx.settings into the geant settings, then override what you need. If you have your own geant app_settings you could run update_settings twice.

Either make an instance of type ListAppSetting or convert a list of dicts of settings via ListAppSetting(list_of_dicts).

Is more docs needed?

I'll play around with it, see if I actually want it 😅 . On one hand I want argus_htmx to tell me what settings it wants (additional apps, middleware, context processors, etc). This also changes regularly (additional context processors for example) and I don't want to copy everything. But I also want some more granular control (swap out our own context processor for theming or whatever).

elfjes avatar Oct 21 '24 10:10 elfjes

I'm having some trouble with argus-htmx depending on stuff from the spa-settings that it doesn't actually need...

hmpf avatar Oct 21 '24 10:10 hmpf

Do docs/README in https://github.com/Uninett/argus-htmx-frontend need a cleanup round after this PR is merged?

Yes! We need a release of argus-server, "vN", with this, then a release of argus-frontend-htmx that depends on "vN" and that has its readme updated with new instructions. I'll make an issue.

hmpf avatar Oct 23 '24 13:10 hmpf