WIP: jitsimeet #176
No reviewers
Labels
No labels
bug
duplicate
enhancement
help wanted
invalid
question
security
wontfix
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: evolix/ansible-roles#176
Loading…
Reference in a new issue
No description provided.
Delete branch "jitsimeet"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
As discussed with @gcolpart during our last meeting (Tuesday, March 19), I'm opening this pull request to begin the work of merging my various Ansible code branches for deploying Evolix Services into the unstable branch, starting with jitsimeet.
There's not much to say...
Variable names should be prefixed with the role's name.
I would rename like this :
I didn't spot any big issue. And I guess y'ouve tried the playbook many times.
But some parts might be improved slightly to match idioms and conventions.
@ -0,0 +1,205 @@
---
# tasks file for jitsimeet install
- name: Set FQDN
I'm pretty sure there is a native module for setting the hostname
@ -0,0 +5,4 @@
command: "hostnamectl set-hostname {{ domains | first }}"
- name: Add Prosody apt repository key
ansible.builtin.get_url:
We usually prefer to download the file into the ansible role ans use a simple copy in the role. That way we don't depend on the availability of the file when deploying.
@ -0,0 +12,4 @@
force: true
- name: Add Jitsi Meet apt repository key + dearmor hack
shell: curl -sL https://download.jitsi.org/jitsi-key.gpg.key | sh -c 'gpg --dearmor > /etc/apt/trusted.gpg.d/jitsimeet.gpg'
We don't store the key there anymore. We will find examples of how we do this in many other roles in ansible-roles.git
@ -0,0 +16,4 @@
- name: Add Prosody apt repository
ansible.builtin.apt_repository:
repo: "deb [signed-by=/etc/apt/trusted.gpg.d/prosody.gpg] https://packages.prosody.im/debian {{ ansible_distribution_release }} main"
You should specify the name of the file.
Also you should deal with the format (plain old one-liner, or deb822) dependanting on your Debian version.
You'll find examples of this if you search for
deb822
in the repository.@ -0,0 +102,4 @@
- name: Add bloc to jicofo.conf to disable sctp
ansible.builtin.blockinfile:
path: /etc/jitsi/jicofo/jicofo.conf
marker: "# {mark} ANSIBLE MANAGED BLOCK"
If you use a generic marker, you might keep the default one.
But if you think there might be more than one block in the future, you should use a more descriptive marker.
@ -0,0 +110,4 @@
}
- name: Unregister default jvb account in prosody
ansible.builtin.command: prosodyctl unregister jvb auth.{{ domains | first }}
You should use a syntax like this :
@ -0,0 +116,4 @@
ansible.builtin.command: prosodyctl register jvb auth.{{ domains | first }} {{ jitsi_meet_jvb_secret }}
- name: Restart prosody
ansible.builtin.service:
We tend to use
systemd
instead of the more genericservice
module. It's less compatibe but more specfic, which is alright in our Debian-centric context.@ -0,0 +118,4 @@
- name: Restart prosody
ansible.builtin.service:
name: prosody
state: restarted
Maybe, this shoud become a
notify
and a handler, to trigger the restart only if changes require a restart have happened.@ -0,0 +146,4 @@
src: "/etc/nginx/sites-available/{{ domains |first }}.conf"
dest: "/etc/nginx/sites-enabled/{{ domains |first }}.conf"
state: link
- name: Reload nginx conf
You should use
notify
and a handler.@ -0,0 +156,4 @@
state: directory
mode: '0755'
- name: Generate certificate with certbot
shell: certbot certonly --webroot --webroot-path /var/lib/letsencrypt --non-interactive --agree-tos --email {{ certbot_admin_email }} -d {{ domains |first }}
Check if
command
is usable here (I guess it is, since there is no pipe or redirection).command
is more safe thanshell
.@ -0,0 +172,4 @@
- { src: 'nginx/vhost.conf.j2', dest: "/etc/nginx/sites-available/{{ domains |first }}.conf" }
- { src: 'nginx/multiplex.conf.j2', dest: '/etc/nginx/modules-available/multiplex.conf' }
- name: Enable multiplex module conf
Add a
notify
@ -0,0 +178,4 @@
dest: '/etc/nginx/modules-enabled/multiplex.conf'
state: link
- name: Enable nginx vhost
Add a
notify
@ -0,0 +184,4 @@
dest: "/etc/nginx/sites-enabled/{{ domains |first }}.conf"
state: link
- name: Reload nginx conf
replace this with a
ansible.builtin.meta: flush_handlers
@ -0,0 +195,4 @@
register: ssl_coturn
- name: Generate certificate for coturn with certbot
shell: certbot certonly --webroot --webroot-path /var/lib/letsencrypt --non-interactive --deploy-hook /etc/letsencrypt/renewal-hooks/deploy/coturn-certbot-deploy.sh --agree-tos --email {{ certbot_admin_email }} -d {{ turn_domains |first }}
replace with
command
if possible@ -0,0 +1,71 @@
---
# tasks file for other domains if any
- name: Template config files
you could put
owner
/group
/mode
in the task, instead of the loop. It's better fr readability.And if we need to add a file with different values, we can refactor later differently.
@ -0,0 +15,4 @@
- name: Check if SSL certificate is present and register result
stat:
path: "/etc/letsencrypt/live/{{ domain }}/fullchain.pem"
register: ssl
you should use a more specific variable name to avoid collision. For this kind of very local usage.
And since Ansible has no notion of variable scope, we tend to prefix this kinf of variable with an underscore, to show that it's intended to be used locally.
@ -0,0 +25,4 @@
"$daemon_cert_root/$domain.key"
service coturn restart >/dev/null
;;
maybe add a comment to clarify why we only deal with the main domain, and what should be done (if any) for others.
With this new commit I add...
jitsimeet_
prefix to relevant variable namesansible.builtin.
prefix to module namesThen I deployed and tested.
Good news: I did not encounter any problem. 😄
More to come...
With this commit I changed...
files/
)ansible.builtin.service
=>ansible.builtin.systemd
ansible.builtin.shell
=>ansible.builtin.command
With this commit I changed...
ansible.builtin.shell
=>ansible.builtin.command
jitsimeet_ prefix
to variable names to avoid collisionWith this commit I added handlers.
0d9a15447a
to6f387b029c
Checkout
From your project repository, check out a new branch and test the changes.Merge
Merge the changes and update on Forgejo.