-
Notifications
You must be signed in to change notification settings - Fork 0
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 management command for generating configuration docs #9
Conversation
5285bbf
to
509cd77
Compare
fc2005e
to
9e93586
Compare
800c0ac
to
5616e91
Compare
5616e91
to
b7a5383
Compare
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.
Thanks for porting the changes in OIP to the library! @pi-sigma
I have some minor comments (mostly optional suggestions), it looks good to me overall 👍
django_setup_configuration/management/commands/generate_config_docs.py
Outdated
Show resolved
Hide resolved
tests/mocks.py
Outdated
@@ -0,0 +1 @@ | |||
mock_product_doc = '.. _product:\n\n=====================\nProduct Configuration\n=====================\n\nSettings Overview\n=================\n\nEnable/Disable configuration:\n"""""""""""""""""""""""""""""\n\n::\n\n PRODUCT_CONFIG_ENABLE\n\nRequired:\n"""""""""\n\n::\n\n PRODUCT_NAME\n PRODUCT_SERVICE_URL\n\nAll settings:\n"""""""""""""\n\n::\n\n PRODUCT_NAME\n PRODUCT_SERVICE_URL\n PRODUCT_TAGS\n\nDetailed Information\n====================\n\n::\n\n Variable PRODUCT_NAME\n Setting Name\n Description The name of the product\n Possible values string\n Default value No default\n \n Variable PRODUCT_SERVICE_URL\n Setting Service url\n Description The url of the service\n Possible values string (URL)\n Default value No default\n \n Variable PRODUCT_TAGS\n Setting tags\n Description Tags for the product\n Possible values string, comma-delimited (\'foo,bar,baz\')\n Default value example_tag\n' # noqa |
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 think we can use triple quotes to make it more readable:
mock_product_doc = """
.. _product:
=====================
Product Configuration
=====================
Settings Overview
=================
Enable/Disable configuration:
"""""""""""""""""""""""""""""
::
PRODUCT_CONFIG_ENABLE
Required:
"""""""
::
PRODUCT_NAME
PRODUCT_SERVICE_URL
All settings:
"""""""""""
::
PRODUCT_NAME
PRODUCT_SERVICE_URL
PRODUCT_TAGS
Detailed Information
====================
::
Variable PRODUCT_NAME
Setting Name
Description The name of the product
Possible values string
Default value No default
Variable PRODUCT_SERVICE_URL
Setting Service url
Description The url of the service
Possible values string (URL)
Default value No default
Variable PRODUCT_TAGS
Setting tags
Description Tags for the product
Possible values string, comma-delimited ('foo,bar,baz')
Default value example_tag
""" # noqa
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.
@stevenbal I haven't been able to get the mock to work in more readable format. I kept getting errors due to differences in whitespace and I couldn't eliminiate the issue.
django_setup_configuration/management/commands/check_config_docs.py
Outdated
Show resolved
Hide resolved
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.
Correct me if I'm wrong, but now ConfigSettingsModel
is completely decoupled from BaseConfigurationStep
. So now you need to write the lists of required settings twice - in ConfigSettingsModel
and in BaseConfigurationStep
?
It is different from the implementation in Open Inwoner, where ConfigSettingsModel instance is reused in configuration steps. I think it's a better approach and I think there would be a relation between these classes.
Also you tightly couple models fields and env var settings. I'm not sure about it, since in the future we plan to have configuration steps with bulk-update of several model fields.(@stevenbal is working on such configuration for Open Zaak authorizations now)
Should there be an option for such cases? But it can be added in the next iteration if it's complicated
django_setup_configuration/management/commands/generate_config_docs.py
Outdated
Show resolved
Hide resolved
django_setup_configuration/management/commands/generate_config_docs.py
Outdated
Show resolved
Hide resolved
django_setup_configuration/management/commands/check_config_docs.py
Outdated
Show resolved
Hide resolved
485138b
to
8d96e01
Compare
- delete registry, store config_settings as class_attributes on configuration steps - add static method to update model field descriptions with custom fields - modify template to show required settings section only if required settings are present
ef81ef0
to
fe6e798
Compare
c6d70cb
to
424304b
Compare
django_setup_configuration/base.py
Outdated
Add custom fields + descriptions defined in settings to | ||
`basic_field_descriptions` | ||
""" | ||
custom_fields = getattr(settings, "DJANGO_SETUP_CONFIG_CUSTOM_FIELDS", None) |
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.
What is the shape of the setting and what does it do? It's never used but this line of code. I think this class was designed to specify config step settings and related help texts, so it's the best place for custom help texts if they are needed.
from .exceptions import PrerequisiteFailed | ||
|
||
|
||
class BaseConfigurationStep(ABC): | ||
verbose_name: str | ||
required_settings: list[str] = [] | ||
enable_setting: str = "" | ||
config_settings: ConfigSettingsModel |
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.
You specify it here but it's never used
django_setup_configuration/base.py
Outdated
@@ -0,0 +1,174 @@ | |||
from dataclasses import dataclass |
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 just noticed that the file is called base
. I find it confusing. Could you rename it into something more specific please?
testapp/configuration.py
Outdated
models=[User], | ||
file_name="user", | ||
namespace="USER_CONFIGURATION", | ||
excluded_fields=["id", "date_joined", "is_active", "last_login"], |
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'll put here a comment about the implementation in general, because I think this line provides a good example.
Why do we need to set up excluded_fields
when we already specify required_settings
? I think it's unnecessary.
With current implementation we need to specify settings twice: as env vars in the configuration step and as model fields in config settings. So it looks like config settings make things not easier, but the opposite.
Perhaps we have different visions of what the config setting should do.
I'll specify mine here. We can discuss it together with @stevenbal .
So I thought config settings class would be used to:
- specify settings used and required for the particular config step (so no need to specify them in the config step itself)
- retrieve help text for them when possible from related model fields
- helps to generate document in .rst for the particular config step
For now config setting class is used to retrieve help text for all model fields of the particular model.
I don't quite understand why it is needed. Why loop on the all model fields and not on the settings?
The model can have a lot of fields and we can change only one of them.
So my main issue is the same as in my previous review - I'd like config-settings to be more coupled with config_step and less with the model.
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.
Discussed, @annashamray to provide an example on how to make this better in her eyes, can be discussed with @stevenbal
Discussed with Paul and Sidney. The tight-couping of settings with models and the resulting documentation in the case of OIP is on purpose and deemed workable, if setup-configuration is meant to be used differently then we'll keep our implementation as-is. Issue can be discussed by TeamBron during refinement as Paul is on holiday for the next 2 weeks. |
As discussed here is an example how I see it:
in the Here is how UserConfigurationStep would look:
I hope my point is clearer now |
- replace blacklist (`exclude`) with explicit whitelist for determining target model fields - avoid use of private Django API for skipping relational fields (made possible by avoiding blacklist) - add support for manually adding documentation for settings which are not associated with any model field
424304b
to
33a4532
Compare
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.
Good work! And thanks you for the thorough code documentation.
I have minor remarks, otherwise it's very nice!
django_setup_configuration/management/commands/generate_config_docs.py
Outdated
Show resolved
Hide resolved
] | ||
|
||
# additional requirements from related configuration steps | ||
related_steps = [step for step in getattr(config_step, "related_steps", [])] |
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 don't think we have related_steps
variable/property in BaseConfigurationStep
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.
It's not required (hence the getattr
), but it can be used to embed the documentation for several configuration steps in another one. For example, if you have a config step for Foo
, which has two API services Bar
and Baz
associated with it, I don't want three documentation files foo_doc.rst
, bar_doc.rst
, baz_doc.rst
. I want a single file that contains the documentation for everything in one place; otherwise the docs get cluttered and harder to process.
Example use case in OIP (the PR does not reflect the recent changes to the library): https://github.com/maykinmedia/open-inwoner/blob/1cb83262cfe7ef29e5816c96ebdb51b78bfa73d1/src/open_inwoner/configurations/bootstrap/zgw.py#L341
I've updated the comment to make the intention more clear. Let me know if this makes sense, otherwise I'll have to think of a different solution for our case in OIP.
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.
Thanks for explanation and the example. I think of two options how to make it a little clearer:
- Declare
related_steps
variable to BaseConfigurationStep, it can have default as empty list. So it would be added to the explicit API of the class. - Am I correct that
ConfigurationStep.related_steps
variable is used only to generate documentation now? If this is true andrelated_steps
is not planned to use in the logic of configuration steps, this variable could be moved toConfigSettings
class asrelated_config_settings
Anyway the explicit declaration of the variable with the typehint would be appreciated
django_setup_configuration/templates/django_setup_configuration/config_doc.rst
Show resolved
Hide resolved
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.
Mostly minor comments
django_setup_configuration/management/commands/generate_config_docs.py
Outdated
Show resolved
Hide resolved
django_setup_configuration/management/commands/generate_config_docs.py
Outdated
Show resolved
Hide resolved
af06d8f
to
9911b82
Compare
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.
Almost there :)
self.create_config_fields(model=model) | ||
|
||
@staticmethod | ||
def get_default_value(field: models.Field) -> str: |
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.
Pycharm doesn't complain here
] | ||
|
||
# additional requirements from related configuration steps | ||
related_steps = [step for step in getattr(config_step, "related_steps", [])] |
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.
Thanks for explanation and the example. I think of two options how to make it a little clearer:
- Declare
related_steps
variable to BaseConfigurationStep, it can have default as empty list. So it would be added to the explicit API of the class. - Am I correct that
ConfigurationStep.related_steps
variable is used only to generate documentation now? If this is true andrelated_steps
is not planned to use in the logic of configuration steps, this variable could be moved toConfigSettings
class asrelated_config_settings
Anyway the explicit declaration of the variable with the typehint would be appreciated
:: | ||
|
||
{% spaceless %} | ||
{{ enable_settings }} |
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.
Does template variable enable_settings
work for you?
I create user.rst
documentation for testapp and this setting is rendered as empty
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.
No, it didn't work. I've deleted the testapp template and use the standard template for testing to avoid this kind of confusion.
6bbe296
to
5629f83
Compare
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.
Good work!
🥳 thanks for the combined effort for this PR! |
"including file extension: /absolute/path/to/image.png" | ||
), | ||
models.IntegerField: "string representing an integer", | ||
models.JSONField: "Mapping: {example}".format(example="{'some_key': 'Some value'}"), |
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.
could also be a string, float, int, None or list... 😬
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.
Split remark off into a separate issue
The PR adds management commands for generating and testing documentation for the configuration setup steps. The docs describe which setting options are available/required, what are the possible values and default values.
Closes #8
Based on Open Inwoner #2297