App Config Schema #213
App Config Schema #213
Conversation
snaselj
requested review from
whitej6,
itdependsnetworks and
jdrew82
as code owners
| "cisco_ios": { | ||
| "type": "string", | ||
| "default": {} | ||
| }, | ||
| "arista_eos": { | ||
| "type": "string", | ||
| "default": {} | ||
| } | ||
| }, |
There was a problem hiding this comment.
This is an example and not the schema, this setting is only {} be default
tasks.py
Outdated
| @task | ||
| def generate_app_config_schema(context): | ||
| """Generate the app config schema based on the app config.""" | ||
| start(context, service="nautobot") | ||
| nbshell(context, file="development/app_config_schema.py", env={"APP_CONFIG_SCHEMA_COMMAND": "generate"}) | ||
|
|
||
|
|
||
| @task | ||
| def validate_app_config(context): | ||
| """Validate the app config based on the app config schema.""" | ||
| start(context, service="nautobot") | ||
| nbshell(context, plain=True, file="development/app_config_schema.py", env={"APP_CONFIG_SCHEMA_COMMAND": "validate"}) |
There was a problem hiding this comment.
Does this have to run via nbshell since it's importing the app config?
There was a problem hiding this comment.
@lampwins I think there may be efforts to explore adding this to startup and being done by core right?
There was a problem hiding this comment.
To import the App config class, it's necessary to do it in initialized Nautobot / Django environment. It is possible to use AST parser instead, but this seems to be a more simple solution.
Another step is to deprecate default_settings and required_settings config class properties, and replace it with JSON schema. For Nautobot 2.x, it can work the way, that when the schema file is present, it will be used and deprecation message will be logged out. In Nautobot 3.x, we can remove these properties.
Another improvement I suggest, is to store the App configuration as a property of it's config class, e.g. NautobotAppConfig.settings. All apps code then can use this, instead of accessing django.PLUGINS_CONFIG.
| def _enrich_object_schema(schema, defaults, required): | ||
| schema["additionalProperties"] = False | ||
| for key, value in schema["properties"].items(): | ||
| if required and key in required: | ||
| value["required"] = True | ||
| default_value = defaults and defaults.get(key, None) | ||
| if value["type"] == "object" and "properties" in value: | ||
| _enrich_object_schema(value, default_value, None) | ||
| elif default_value is not None: | ||
| value["default"] = default_value |
There was a problem hiding this comment.
@lampwins FYI apps is looking into providing as part of the dev env the ability to dynamically generate the json schema based on the required and default settings in the app config. The schema is held in the root of the package app-config-schema.json.
Also in going through this I have a suspicion that our heavy use of parsing the app config in a constance.py breaks lazy loading. For MOST scenarios this is not problematic but it breaks some out of the box behaviors for django when using the override setting decorator.
| check-migrations: | ||
| needs: | ||
| - "bandit" | ||
| - "ruff" | ||
| - "flake8" | ||
| - "poetry" | ||
| - "yamllint" | ||
| - "black" | ||
| runs-on: "ubuntu-22.04" | ||
| strategy: | ||
| fail-fast: true | ||
| matrix: | ||
| python-version: ["3.11"] | ||
| nautobot-version: ["2.0.0"] | ||
| env: | ||
| INVOKE_NAUTOBOT_FIREWALL_MODELS_PYTHON_VER: "${{ matrix.python-version }}" | ||
| INVOKE_NAUTOBOT_FIREWALL_MODELS_NAUTOBOT_VER: "${{ matrix.nautobot-version }}" | ||
| steps: | ||
| - name: "Check out repository code" | ||
| uses: "actions/checkout@v4" | ||
| - name: "Setup environment" | ||
| uses: "networktocode/gh-action-setup-poetry-environment@v4" | ||
| - name: "Set up Docker Buildx" | ||
| id: "buildx" | ||
| uses: "docker/setup-buildx-action@v3" | ||
| - name: "Build" | ||
| uses: "docker/build-push-action@v5" | ||
| with: | ||
| builder: "${{ steps.buildx.outputs.name }}" | ||
| context: "./" | ||
| push: false | ||
| load: true | ||
| tags: "${{ env.APP_NAME }}/nautobot:${{ matrix.nautobot-version }}-py${{ matrix.python-version }}" | ||
| file: "./development/Dockerfile" | ||
| cache-from: "type=gha,scope=${{ matrix.nautobot-version }}-py${{ matrix.python-version }}" | ||
| cache-to: "type=gha,scope=${{ matrix.nautobot-version }}-py${{ matrix.python-version }}" | ||
| build-args: | | ||
| NAUTOBOT_VER=${{ matrix.nautobot-version }} | ||
| PYTHON_VER=${{ matrix.python-version }} | ||
| - name: "Copy credentials" | ||
| run: "cp development/creds.example.env development/creds.env" | ||
| - name: "Checking: App Config" | ||
| run: "poetry run invoke validate-app-config" |
There was a problem hiding this comment.
This change should defer to dev standards. I understand why but we lose some high level visibility when combining 3 tests into one CI task
| "$schema": "https://json-schema.org/draft/2020-12/schema", | ||
| "$id": f"https://raw.githubusercontent.com/{owner}/{repository}/develop/{package_name}/app-config-schema.json", | ||
| "$comment": "TBD: Update $id, replace `develop` with the future release tag", | ||
| **SchemaBuilder().to_json_schema(app_config), # type: ignore |
There was a problem hiding this comment.
So we're generating the schema based on the example provided in development/nautobot_config.py? That seems fragile to me - depending on the app I'd think that the development config often may not match the full configuration possible for the app.
There was a problem hiding this comment.
Correct, this invoke task is just a helper to quickly get something from the current configuration. Needs to be updated by the user.
There was a problem hiding this comment.
Let's make sure that's documented very clearly then. :-)
There was a problem hiding this comment.
Added warning statement to invoke task docstring and stdout.
|
I think it would be good to get the testing structure in place so as the schema get's more complicated, we can just provide mock valid and mock invalid configs for regression testings. |
Agreed, I would postpone to create such test to the stage when the validation will be done in the app, and implement that in unittest. Currently, we are preparing JSON schemas only, without real usage in app and validation is in invoke task only. |
|
Seems fair |
Closes: NaN
What's Changed
invoke generate-app-config-schemacommand to generate a JSON schema for the App config.invoke validate-app-configcommand to validate the App config against the schema.pylint,check-migrationsandvalidate-app-configto single CI Jobcheck-in-docker.