Feature/114 add django setup configuration #115
Conversation
Coperh
force-pushed
the
feature/114-add-django-setup-configuration
branch
10 times, most recently
from
fc242eb
to
c9f80ed
Compare
Coperh
force-pushed
the
feature/114-add-django-setup-configuration
branch
2 times, most recently
from
c60c2e3
to
6a447fc
Compare
Coperh
force-pushed
the
feature/114-add-django-setup-configuration
branch
from
6e001b1
to
aca7802
Compare
|
Mostly copied Example of it being used here open-zaak/open-zaak#1728 |
Coperh
force-pushed
the
feature/114-add-django-setup-configuration
branch
from
20f406b
to
aca9492
Compare
|
I am also unsure about the package/file names |
There was a problem hiding this comment.
I took a preliminary glance, haven't looked into much details yet, but I have some concerns - perhaps about the way django-setup-configuration itself works:
amount of settings
there are a lot of settings being defined purely to facilitate bootstrapping configuration. Some of these setting names also have a big potential to cause confusion with settings that actually have some meaning at runtime. I don't think this will improve the developer experience
settings datatypes
I noticed in OIP that settings are used so that envvars can be used to specify them, but the datatypes of these settings (like lists of strings, periods/comma's having special meaning...) but also dict/mappings complicates passing them via the env
you basically have a Helm chart or Ansible playbook where the datatype is properly defined in YAML, this is then templated out to a string so it can be passed as an envvar, after which the application deserializes it again from a string into the expected datatype
I would much prefer one of the options below (with the first having my preference):
- load a YAML/TOML/JSON file that contains the data/config to initialize. you then only need to point to which file to load, plus it will scale in the future when we don't have solo models anymore but instead regular models. it's basically a django-agnostic fixture format
- read the values directly from the env and remove the need to define them as settings. the library knows how to interpret each envvar and parse it into the expected data-structure
IMO the former approach makes documenting the format a lot easier, you can simply provide an example template, you can add comments if you use YAML/TOML and the processing step of that file is able to populate missing information (the endpoints!) from the discovery endpoint if that's all that is specified.
|
FYI for Conor, Paul is working on a related issue: https://taiga.maykinmedia.nl/project/open-inwoner/issue/2563 |
Coperh
force-pushed
the
feature/114-add-django-setup-configuration
branch
2 times, most recently
from
92960f5
to
68d3a61
Compare
Coperh
force-pushed
the
feature/114-add-django-setup-configuration
branch
2 times, most recently
from
3bc045b
to
93f6c66
Compare
| all_settings = { | ||
| "sync_groups": config.sync_groups, | ||
| "oidc_use_nonce": config.oidc_use_nonce, | ||
| } |
There was a problem hiding this comment.
Is there a way to use default values for boolean fields if its empty? Otherwise they always default to false.
There was a problem hiding this comment.
I think you can use the __init__ in the form to set a default? self.fields["sync_groups"].default = False? But maybe I'm not understanding the question right.
|
Have reimplemented it in OZ with the new format: open-zaak/open-zaak#1728 |
tests/setupconfig/test_auth.py
Outdated
| ) | ||
| assert config.username_claim == ["claim_name"] | ||
| assert config.groups_claim == ["groups_claim_name"] | ||
| assert config.claim_mapping == {"first_name": "given_name"} |
There was a problem hiding this comment.
actually, this bug should really be caught by form validation 🤔
| assert not config.is_configured() | ||
|
|
||
| config.configure() | ||
|
|
||
| assert config.is_configured() |
There was a problem hiding this comment.
Can you explain what this actually means/implies?
There was a problem hiding this comment.
IDK tbh. Copied it from open-iwoner
Coperh
force-pushed
the
feature/114-add-django-setup-configuration
branch
from
54ad63f
to
3aac68c
Compare
fixes: #114
Implements django setup configuration in OIDC itself