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

Add support for Frigate #79

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

cvwright
Copy link

@cvwright cvwright commented Jul 8, 2023

This PR adds initial support for the Frigate network video recorder https://github.com/blakeblackshear/frigate

@cvwright cvwright marked this pull request as draft July 8, 2023 22:28
@spantaleev
Copy link
Member

Thanks for contributing support for Frigate!

Here's some feedback:

  • it'd be nice to add a documentation page
  • it'd be nice to expose it through Traefik like all other services. When done, frigate_container_http_host_bind_port should be unset. Traefik can also be used in local-only mode and without SSL certificates, so it shouldn't be preventing anyone from using Frigate in a local-only environment.
  • environment variables are defined inline in the systemd service file. It's better to use an env file which is passed via --env-file. See an example here. This has 4 benefits:
    • systemd will not need to be reloaded when the environment variables change (the playbook reloads it automatically anyway, so it's not that big of a deal)
    • any sensitive data in environment variables would not be visible from every user the machine doing ps aux
    • the systemd service file will be cleaner
    • adding a variable for extending the env file will be possible and easy
  • in tasks/install.yml, it's better to keep related actions close to one another. The file currently starts with creating a Docker network, then does a bunch of file-related stuff, then goes back to pulling Docker images. Most roles do all Docker-related operations at the bottom of the file.
  • defaults/main.yml mixes capitalization for booleans. It sometimes uses false, other times `False. It's better and more consisent with the rest of the playbook by using lowercase everywhere

@cvwright
Copy link
Author

cvwright commented Jul 9, 2023

Thanks @spantaleev! It's great to get feedback from the expert.

The latest commits address most of your comments. The big to-do item that remains is to add support for Traefik.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants