Find a new place to document properties in settings.yml
#1573
Assignees
Labels
Comments
jotaen4tinypilot
added a commit
that referenced
this issue
Aug 23, 2023
Part of #1460. This PR templatizes the Janus main config file, and adds a privileged script for (re-)generating the config from the template plus from the properties in `settings.yml` (i.e., `ustreamer_janus_stun_server` and `ustreamer_janus_stun_port`). ## Notes - In the Debian `postinst` routine, we deliberately delegate to a privileged script instead of just inlining the logic ([like we do for writing the app settings](https://github.com/tiny-pilot/tinypilot/blob/e3054409299950a8698c5c1dc4ca9e8c7a8bcdd6/debian-pkg/debian/tinypilot.postinst#L73-L80), for example), since we want to re-use the `configure-janus` logic in the backend in a later step. - Later, when calling the `configure-janus` privileged script from the backend, we also need to restart the uStreamer and Janus systemd services manually. To me, it felt safer and more explicit to let the caller take care of that, instead of making the service restart part of `configure-janus`. I also wasn’t sure we should be issuing direct calls to `systemctl` or `/usr/sbin/service` anyways during the Debian install procedure, instead of using `deb-systemd-invoke`. - I debated whether we should add a comment above the `nat {}` block in the Janus config, however, I wasn’t sure whether a generic explanation of what STUN is would add enough value. - Since neither `ustreamer_janus_stun_server` nor `ustreamer_janus_stun_port` do have default values, it doesn’t make sense to list them [in `settings.py`](https://github.com/tiny-pilot/tinypilot/blob/e3054409299950a8698c5c1dc4ca9e8c7a8bcdd6/app/update/settings.py#L24-L31). We currently don’t have an authoritative place anymore that lists and documents all supported properties in `settings.yml`, so the two newly added STUN properties are implicit and undocumented now. I have opened #1573 for tracking that, since I think this is a broader issue. A few more things on testing: - I have tested this PR quite a bit on device with the [latest bundle build off `af5ec0d`](https://output.circle-artifacts.com/output/job/29dc16fa-0b35-4b41-b920-85e61fa8883f/artifacts/0/bundler/dist/tinypilot-community-20230822T1618Z-1.9.0-66+af5ec0d.tgz), so if you like, I’d suggest that a brief spot-check QA on your end would suffice. - Please note that testing this PR is only about verifying that we generate a valid Janus config [according to the documentation](https://github.com/meetecho/janus-gateway/blob/7c7093e1885834ac4154244bd99f8dd93ae71170/conf/janus.jcfg.sample.in#L259-L288), not about actually verifying the STUN mechanism itself, as the setup for that [is quite complex and laborious](tiny-pilot/tinypilotkvm.com#939 (comment)). We already gathered enough evidence that Janus with STUN in itself does the job, and solves the use-case scenarios we are after. - We [have an issue](#1578) where Janus would fail to come up right after booting, if a STUN server is specified. We’ll fix this separately. <a data-ca-tag href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1579"><img src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review on CodeApprove" /></a>
jotaen4tinypilot
added a commit
that referenced
this issue
Nov 30, 2023
Resolves #1524. The outcome of our discussion in the ticket was this: - The SQlite DB (`tinypilot.db`) should be the default place for new settings, whereas the YAML file (`settings.yml`) should only be used in exceptional cases, and obviously for legacy reasons. - We favor having a single storage solution over a dual approach, as that simplifies future decision-making, and eliminates potential migration burden between the two storage formats. - The SQlite DB doesn’t easily allow programmatic access right now (in contrast to the YAML file), so we [created this ticket for removing this hurdle](#1690). - We [will document the parameters in `settings.yml` separately](#1573). This PR adds a comment in `settings.py`, marking its usage as quasi-deprecated. It also adds some historical context, references the SQlite DB as default storage, and lists sample exceptional cases. I realized that we probably don’t need to add any documentation to the SQlite DB, since there is not really an alternative to it any longer. So by deprecating `settings.yml`, the SQlite DB is the only storage option anyway, and there shouldn’t be any ambiguity left. Contrary to [my initial proposal](#1524 (comment)), I also don’t think we should move `settings.py` up to the `app` package, as that might mistakenly promote its usage. <a data-ca-tag href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1692"><img src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review on CodeApprove" /></a> --------- Co-authored-by: Jan Heuermann <[email protected]>
jdeanwallace
added a commit
that referenced
this issue
Apr 19, 2024
Resolves #1573 Since winning the war on Ansible, we've dropped support for many TinyPilot settings that were configurable via `/home/tinypilot/settings.yml`. Apart from this blog post ([/whats-new-in-2022-05/](https://github.com/tiny-pilot/tinypilotkvm.com/blob/ba842c50235005fb9253e316cf4f85a543548634/content/blog/whats-new-in-2022-05/index.md)) we haven't really documented (publicly) which settings are supported, deprecated, and unsupported. This PR does just that: * Document preview: [docs/user-configurable-settings.md](https://github.com/tiny-pilot/tinypilot/blob/document-user-configurable-settings/docs/user-configurable-settings.md) ### Notes 1. The list of [supported settings](https://github.com/tiny-pilot/tinypilot/blob/document-user-configurable-settings/docs/user-configurable-settings.md#supported-settings), were compiled from * [app/update/settings.py#L76-L86](https://github.com/tiny-pilot/tinypilot-pro/blob/78269651ca646746b7b55d83d19b377a943ec7de/app/update/settings.py#L76-L86) 3. The list of [deprecated settings](https://github.com/tiny-pilot/tinypilot/blob/document-user-configurable-settings/docs/user-configurable-settings.md#deprecated-settings-legacy), were compiled from a combination of * [debian-pkg/opt/ustreamer-launcher/launch#L95-L101](https://github.com/tiny-pilot/tinypilot-pro/blob/78269651ca646746b7b55d83d19b377a943ec7de/debian-pkg/opt/ustreamer-launcher/launch#L95-L101) * "Unsupported vars" from tiny-pilot/tinypilot-pro#972 (comment) 4. The list of [unsupported settings](https://github.com/tiny-pilot/tinypilot/blob/document-user-configurable-settings/docs/user-configurable-settings.md#unsupported-settings-non-configurable), were compiled from a combination of * [2.6.0/ansible-role/defaults/main.yml](https://github.com/tiny-pilot/tinypilot-pro/blob/2.6.0/ansible-role/defaults/main.yml) * [2.6.0/ansible-role-ustreamer/defaults/main.yml](https://github.com/tiny-pilot/tinypilot-pro/blob/2.6.0/ansible-role-ustreamer/defaults/main.yml) * [blog/whats-new-in-2022-05/index.md#enabling-h264-video](https://github.com/tiny-pilot/tinypilotkvm.com/blob/ba842c50235005fb9253e316cf4f85a543548634/content/blog/whats-new-in-2022-05/index.md#enabling-h264-video) ### Resources used * https://tinypilotkvm.com/pro/changes#261 * tiny-pilot/tinypilot-pro#972 * [blog/whats-new-in-2022-05/](https://github.com/tiny-pilot/tinypilotkvm.com/blob/ba842c50235005fb9253e316cf4f85a543548634/content/blog/whats-new-in-2022-05/index.md) * [2.6.0/ansible-role/defaults/main.yml](https://github.com/tiny-pilot/tinypilot-pro/blob/2.6.0/ansible-role/defaults/main.yml) * [2.6.0/ansible-role-ustreamer/defaults/main.yml](https://github.com/tiny-pilot/tinypilot-pro/blob/2.6.0/ansible-role-ustreamer/defaults/main.yml) <a data-ca-tag href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1788"><img src="https://codeapprove.com/external/github-tag-allbg.png" alt="Review on CodeApprove" /></a>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The properties in
settings.ymlused to be documented via the Ansible roles. However, the Ansible role for tinypilot is already gone, and the ustreamer role will follow.Therefore, I think it would be good to have one central place where all known properties are declared and documented. We should also clearly label the properties based on whether they are
https://tinypilotkvm.com/pro/changes#261 provides the most up-to-date overview of supported settings of user-editable settings.
Related #1524.
The text was updated successfully, but these errors were encountered: