Add provision_server management command
#4975
Conversation
RuthShryock
changed the title
RuthShryock
changed the title
provision_server management command
…y using key-value pairs and add handling for json and boolean values
|
|
||
| class Command(BaseCommand): | ||
| help = ( | ||
| 'Provision server settings including social apps and constance configs' |
There was a problem hiding this comment.
I like the documentation in general, good work. Especially the argument docs. I would add an example command here too, for quick learning.
| } | ||
|
|
||
| social_app, created = SocialApp.objects.get_or_create( | ||
| defaults=social_app_data |
There was a problem hiding this comment.
This doesn't work, you need to have a lookup based on a unique field - maybe name? The first time it works. Then with a different name, I still get "Social app for my_first_name already exists" even though I changed the name.
| social_app=social_app | ||
| ).exists() | ||
|
|
||
| if not social_app_custom_data_exists: |
There was a problem hiding this comment.
I'd probably inline this as it's setting a variable that is used exactly once. Minor, fine-to-ignore, comment.
| if value.startswith('[') and value.endswith(']'): | ||
| try: | ||
| value = json.loads(value) | ||
| value = json.dumps(value) |
There was a problem hiding this comment.
Interesting way to validate json, I imagine it works fine. I don't think it's possible to error in the dumps line, so that can be moved outside of the try, except.
Or actually I think you could achieve the validation with
try:
json.loads(value)
except json.JSONDecodeError ...
you don't really need the value, right? You're just converting to and then from json to validate it I assume.
If you end up keeping this code - I might try to break it out so it's not so nested. I don't typically say one must "Don't repeat yourself" when repeating just twice. But in this case it might help reduce the nesting.
| 'PROJECT_METADATA_FIELDS', | ||
| 'USER_METADATA_FIELDS', | ||
| ]: | ||
| if value.startswith('[') and value.endswith(']'): |
There was a problem hiding this comment.
Is this needed? I know that json technically isn't supposed to start with an array. But this works for me
>>> data = "[1, 2]"
>>> import json
>>> json.loads(data)
[1, 2]
>>> json.dumps(json.loads(data))
'[1, 2]'…ion logic, and add more example documentation
| f'Invalid JSON value for key {key}. {e}' | ||
| ) | ||
| continue | ||
| json.dumps(value) |
There was a problem hiding this comment.
I don't think this does anything, does it? It serializes to a string but the result isn't saved anywhere.
| provider_id = kwargs['provider_id'] | ||
| name = kwargs['name'] | ||
| client_id = kwargs['client_id'] | ||
| secret = os.getenv('SOCIAL_APP_SECRET') or kwargs.get('secret', '') |
There was a problem hiding this comment.
Very minor but if you could do this
kwargs.get('secret', os.getenv('SOCIAL_APP_SECRET', ''))
That would prefer the kwarg over the env var (Seems right to me) 🤷
| try: | ||
| settings = json.loads(settings_json) | ||
| except TypeError: | ||
| raise json.JSONDecodeError |
There was a problem hiding this comment.
I think this is catching the same error and then re-raising it? Why not allow the original exception to be raised? If you want to catch it and add more context, do that. Or leave it alone. Catching and raising without any benefit is obfuscating the error.
| social_app_data = { | ||
| 'provider': provider, | ||
| 'provider_id': provider_id, | ||
| 'name': name, |
There was a problem hiding this comment.
This line can be omitted, you don't need to repeat the args in get_or_create again in defaults. The usage of get_or_create makes a lot of sense though.
Checklist
Description
Automates setup of social applications and constance configurations through running
./manage.py provision_serveralong with specified arguments.Notes
This command has two subcommands
socialappandconfig.Example:
./manage.py provision_server config MFA_ENABLED=False SUPERUSER_AUTH_ENFORCEMENT=Truewill disable MFA and enable authentication for superusers.(The
configsubcommand can take an infinite number of key-value pairs.)