-
Notifications
You must be signed in to change notification settings - Fork 10
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
📝 [open-zaak/open-zaak#1649] Document envvars #425
Conversation
778ce8e
to
6d918da
Compare
abb51f5
to
0126d0f
Compare
src/objects/conf/base.py
Outdated
# TODO this doesn't seem to have any effect, because we do not catch the exception anywhere? | ||
# https://docs.celeryq.dev/en/latest/userguide/configuration.html#task-soft-time-limit | ||
CELERY_TASK_SOFT_TIME_LIMIT = config( |
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.
@annashamray do you know more about this? In the Celery docs, it is mentioned that after this soft time limit a SoftTimeLimitExceeded
is raised, and @Viicos mentioned that this time limit is needed for notifications_api_common
, but I don't see that exception being caught in that library
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.
I've found the related PR (#362) but it's not much of help.
My guess that it is a copy-paste from Open Forms and I also haven't found any catching of SoftTimeLimitExceeded
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.
In that case I'll remove this envvar entirely
0126d0f
to
e6ae8b1
Compare
e6ae8b1
to
50a57a1
Compare
replace the manual envvar docs with OAf `generate_envvar_docs`, this ensure that generic envvars that are used by every component are documented centrally in OAF and custom envvars can be documented in the code where they are declared
50a57a1
to
85b78db
Compare
SITES_CONFIG_ENABLE = config("SITES_CONFIG_ENABLE", default=True) | ||
OBJECTS_DOMAIN = config("OBJECTS_DOMAIN", "") | ||
OBJECTS_ORGANIZATION = config("OBJECTS_ORGANIZATION", "") | ||
SITES_CONFIG_ENABLE = config("SITES_CONFIG_ENABLE", default=True, add_to_docs=False) |
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.
Hmm I feel like a missing opportunity that we won't update django-setup-configuration
config documentation with generate_envvar_docs
command.
Should we run a command to update it also inside bin/generate_envvar_docs.sh
?
Oh maybe in the future we can generate a separate page for them with it?
Do you think this makes sense or I overcomplicate it?
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.
I did not include the setup_configuration envvars yet, because there already was detailed documentation for them and they're not shared between projects. My goal here was mainly to remove the need to duplicate the same help texts for the shared variables.
I do think it would be good to include the setup_configuration envvars as well at some point, though I do think it's best to keep it as a separate page (like it is now) to differentiate between them.
I'm not yet sure what the easiest way to do that, I guess we could indeed run the command with a different --file
and exclude the other groups. We'd probably also have to specify another template for it though, to add the intro text etc. Maybe it should be a separate issue?
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, and we can investigate in the separate issue that maybe Paul's doc generation is enough and covers all our needs?
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.
created maykinmedia/open-api-framework#64 for it
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.
Please create an issue to generate document page for django_setup_configuration settings
Fixes open-zaak/open-zaak#1649 partially
Requires maykinmedia/open-api-framework#22 to be merged and released
Changes