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

Install Janus config files from TinyPilot Debian package #1496

Closed
mtlynch opened this issue Jul 7, 2023 · 5 comments · Fixed by #1552
Closed

Install Janus config files from TinyPilot Debian package #1496

mtlynch opened this issue Jul 7, 2023 · 5 comments · Fixed by #1552
Assignees
Labels
enhancement New feature or request

Comments

@mtlynch
Copy link
Contributor

mtlynch commented Jul 7, 2023

Blocked on https://github.com/tiny-pilot/tinypilot-pro/pull/985

We're currently placing Janus' config files using Ansible:

- name: create Janus config files
template:
src: "{{ item }}.j2"
dest: "{{ ustreamer_janus_configs_dir }}/{{ item }}"
owner: root
group: root
mode: "0644"
loop:
- janus.jcfg
- janus.transport.websockets.jcfg
notify:
- restart Janus

It should be trivial to pull this logic out of Ansible and implement it instead in TinyPilot's Debian package.

Some notes on the migration:

  • The Ansible task is a template task even though the source files don't have any templated values. I don't know if this was a mistake originally or if they used to have templates, but now they're just static files.
    • This makes the task easier, as we should be able to move the files like this:
      • ansible-role-ustreamer/templates/janus.jcfg.j2 -> debian-pkg/etc/janus/janus.jcfg
      • ansible-role-ustreamer/templates/janus.transport.websockets.jcfg.j2 -> debian-pkg/etc/janus/janus.transport.websockets.jcfg
  • The Ansible task templatizes the path based on ustreamer_janus_configs_dir, but we can hardcode the path to /etc/janus for simplicity.
  • We don't have to implement logic for restarting the Janus service.
  • We should preserve root as the user and group.
  • We should preserve 0644 as the permissions, but if that generates a CI error in lintian, we should accept lintian's suggestion. The important thing is that the execute bit is off for everyone and the write bit is off for non-owner.

Similar work

@mtlynch mtlynch added the enhancement New feature or request label Jul 7, 2023
@jdeanwallace
Copy link
Contributor

@mtlynch - Is this a duplicate of tiny-pilot/ustreamer-debian#12?

Do we want to configure Janus in the TinyPilot or uStreamer Debian package?

@mtlynch
Copy link
Contributor Author

mtlynch commented Jul 24, 2023

Do we want to configure Janus in the TinyPilot or uStreamer Debian package?

I'd like to do it in the TinyPilot Debian package because then it's all in a single repo. It's maybe more logical from the uStreamer Debian package, but then we're spreading more of our code across repos, which I think outweighs other concerns.

@mtlynch
Copy link
Contributor Author

mtlynch commented Aug 2, 2023

@db39 - Can you take this on next? This one is similar in process to #1467 but the process is simpler because we can do everything in a single change to the tinypilot repo rather than bouncing between this repo and the ustreamer-debian repo.

@db39
Copy link
Contributor

db39 commented Aug 3, 2023

  • We should preserve root as the user and group.
  • We should preserve 0644 as the permissions, but if that generates a CI error in lintian, we should accept lintian's suggestion. The important thing is that the execute bit is off for everyone and the write bit is off for non-owner.

For handling these permissions, should I use tinypilot.postinst to set them?

@mtlynch
Copy link
Contributor Author

mtlynch commented Aug 3, 2023

@db39 - I don't think we should have to explicitly set the permissions in postinst. I think apt-get just copies over the files with the right permissions, but double check after installing.

@db39 db39 linked a pull request Aug 3, 2023 that will close this issue
@db39 db39 closed this as completed in #1552 Aug 4, 2023
db39 added a commit that referenced this issue Aug 4, 2023
Resolves #1496

This change replaces the Ansible task responsible for creating the Janus
config files, implementing it in TinyPilot's Debian package instead.

To test this bundle, run:

```bash
curl \
  --silent \
  --show-error \
  --location \
  https://raw.githubusercontent.com/tiny-pilot/tinypilot/master/scripts/install-bundle | \
  sudo bash -s -- \
    https://output.circle-artifacts.com/output/job/5e8d773d-c381-465e-bd67-425ff99f5cd3/artifacts/0/bundler/dist/tinypilot-community-20230804T1445Z-1.9.0-46+dd77b49.tgz
```
<a data-ca-tag
href="https://codeapprove.com/pr/tiny-pilot/tinypilot/1552"><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
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants