Skip to content
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

refactor(app): use jsonschema for config #854

Closed

Conversation

paularmstrong
Copy link
Contributor

@paularmstrong paularmstrong commented Feb 27, 2021

Note: looking for feedback, concerns, or agreement before I finish this up

Thinking forward to the point where we want to be able to create the config yaml from the web UI, I think it will be difficult to maintain long-term if we have more than once source of truth for what fields need to exist and how they are validated.

Switching the python application from using voluptuous to jsonschema allows for 3 key benefits:

  1. We can serve the schema from the API to the web app so that it can construct forms without hardcoding any information. The forms and inputs can all be generated based on the schema. Reducing maintenance on config changes would be really nice.
  2. We can build documentation straight from the schema. The current jsonschema to markdown generators that I've found don't quite output human-readable docs, so I'm going to keep looking, otherwise may write one separately, so this is a little longer out. I got a start on this and will keep updating what it looks like here.
  3. Errors on startup with the config validation are more human readable. Error parsing config: 'mqtt' becomes ERROR : deque(['mqtt']) - 'host' is a required property (can probably do better, but this is a start)

There are still a few issues present that need to be fixed, like validating that rtmp and detect roles are present. Some of the more complex validation is just a little harder to write.

@blakeblackshear
Copy link
Owner

This seems like a good direction to move in. It does look like we have some conflicts in with auto-formatting that we need to sync up. If you use the vscode remote container, it should match mine.

@paularmstrong
Copy link
Contributor Author

I can't get used to vscode. It seems you're using Black to format. I installed that and ran it and it looks like it changed a lot more formatting than previously. Do you have some extra settings set somewhere?

@paularmstrong
Copy link
Contributor Author

Here's a start on what the output from my jsonschema-to-markdown will output. It still has a lot of things to deal with.
https://gist.github.com/paularmstrong/d2a7abf76e15bc2a4fbdc549a69fdc94

@blakeblackshear
Copy link
Owner

@paularmstrong
Copy link
Contributor Author

Looks like vscode has two things built-in with black that aren't actually on by default if you're just using black standalone (or in another IDE): --skip-string-normalization and --target-version=py36 (auto-detection default for target version seems to wind up with random results)

@stale
Copy link

stale bot commented Apr 2, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 2, 2021
@stale stale bot removed the stale label Apr 2, 2021
@herostrat herostrat mentioned this pull request Apr 29, 2021
9 tasks
@herostrat
Copy link
Contributor

Is there anything I can help you with, be it coding or review?
I find this change very promising.

@blakeblackshear blakeblackshear deleted the branch blakeblackshear:release-0.9.0 October 5, 2021 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants