WIP: jitsimeet #176

Draft
mgauthier wants to merge 28 commits from jitsimeet into unstable
Owner

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...

  • I did a rebase from unstable just before opening this pull request
  • Only new files are added - all under webapps/jitsimeet
  • Now's a good time to point out all the things that aren't up to Evolix's standards 😸
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... - I did a rebase from _unstable_ just before opening this pull request - Only new files are added - all under _webapps/jitsimeet_ - Now's a good time to point out all the things that aren't up to Evolix's standards 😸
mgauthier added 23 commits 2024-03-20 19:49:12 +01:00
Owner

Variable names should be prefixed with the role's name.

I would rename like this :

  • system_dep → jitsimeet_system_dependencies
  • domains → jitsimeet_domains
  • turn_domains → jitsimeet_turn_domains
  • certbot_admin_email → jitsimeet_certbot_admin_email
  • jitsi_meet_* → jitsimeet_*
Variable names should be prefixed with the role's name. I would rename like this : - system_dep → jitsimeet_system_dependencies - domains → jitsimeet_domains - turn_domains → jitsimeet_turn_domains - certbot_admin_email → jitsimeet_certbot_admin_email - jitsi_meet_* → jitsimeet_*
jlecour requested changes 2024-03-25 07:52:03 +01:00
jlecour left a comment
Owner

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.

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
Owner

I'm pretty sure there is a native module for setting the hostname

I'm pretty sure there is a native module for setting the hostname
mgauthier marked this conversation as resolved
@ -0,0 +5,4 @@
command: "hostnamectl set-hostname {{ domains | first }}"
- name: Add Prosody apt repository key
ansible.builtin.get_url:
Owner

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.

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.
jlecour marked this conversation as resolved
@ -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'
Owner

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

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
mgauthier marked this conversation as resolved
@ -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"
Owner

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.

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.
mgauthier marked this conversation as resolved
@ -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"
Owner

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.

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 }}
Owner

You should use a syntax like this :

ansible.builtin.command:
    cmd: XXX
You should use a syntax like this : ``` ansible.builtin.command: cmd: XXX ```
mgauthier marked this conversation as resolved
@ -0,0 +116,4 @@
ansible.builtin.command: prosodyctl register jvb auth.{{ domains | first }} {{ jitsi_meet_jvb_secret }}
- name: Restart prosody
ansible.builtin.service:
Owner

We tend to use systemd instead of the more generic service module. It's less compatibe but more specfic, which is alright in our Debian-centric context.

We tend to use `systemd` instead of the more generic `service` module. It's less compatibe but more specfic, which is alright in our Debian-centric context.
mgauthier marked this conversation as resolved
@ -0,0 +118,4 @@
- name: Restart prosody
ansible.builtin.service:
name: prosody
state: restarted
Owner

Maybe, this shoud become a notify and a handler, to trigger the restart only if changes require a restart have happened.

Maybe, this shoud become a `notify` and a handler, to trigger the restart only if changes require a restart have happened.
mgauthier marked this conversation as resolved
@ -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
Owner

You should use notify and a handler.

You should use `notify` and a handler.
mgauthier marked this conversation as resolved
@ -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 }}
Owner

Check if command is usable here (I guess it is, since there is no pipe or redirection). command is more safe than shell.

Check if `command` is usable here (I guess it is, since there is no pipe or redirection). `command` is more safe than `shell`.
mgauthier marked this conversation as resolved
@ -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
Owner

Add a notify

Add a `notify`
mgauthier marked this conversation as resolved
@ -0,0 +178,4 @@
dest: '/etc/nginx/modules-enabled/multiplex.conf'
state: link
- name: Enable nginx vhost
Owner

Add a notify

Add a `notify`
mgauthier marked this conversation as resolved
@ -0,0 +184,4 @@
dest: "/etc/nginx/sites-enabled/{{ domains |first }}.conf"
state: link
- name: Reload nginx conf
Owner

replace this with a ansible.builtin.meta: flush_handlers

replace this with a `ansible.builtin.meta: flush_handlers`
mgauthier marked this conversation as resolved
@ -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 }}
Owner

replace with command if possible

replace with `command` if possible
mgauthier marked this conversation as resolved
@ -0,0 +1,71 @@
---
# tasks file for other domains if any
- name: Template config files
Owner

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.

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
Owner

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.

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.
mgauthier marked this conversation as resolved
@ -0,0 +25,4 @@
"$daemon_cert_root/$domain.key"
service coturn restart >/dev/null
;;
Owner

maybe add a comment to clarify why we only deal with the main domain, and what should be done (if any) for others.

maybe add a comment to clarify why we only deal with the main domain, and what should be done (if any) for others.
mgauthier marked this conversation as resolved
mgauthier added 1 commit 2024-03-27 21:55:43 +01:00
Author
Owner

With this new commit I add...

  • the jitsimeet_ prefix to relevant variable names
  • the ansible.builtin. prefix to module names

Then I deployed and tested.
Good news: I did not encounter any problem. 😄

More to come...

With [this new commit](https://gitea.evolix.org/evolix/ansible-roles/pulls/176/commits/41e8f376eeb220526b0379c5a876e11de927a762) I add... - the `jitsimeet_` prefix to relevant variable names - the `ansible.builtin.` prefix to module names Then I deployed and tested. Good news: I did not encounter any problem. 😄 More to come...
mgauthier added 1 commit 2024-03-28 21:08:22 +01:00
Author
Owner

With this commit I changed...

  • how apt sources for Prosody and Jitsi Meet are added (GPG keys now in files/)
  • ansible.builtin.service => ansible.builtin.systemd
  • ansible.builtin.shell => ansible.builtin.command
With [this commit](https://gitea.evolix.org/evolix/ansible-roles/pulls/176/commits/7b3d3764ce235a0761b11558600b3006f7dfe16b) I changed... - how apt sources for Prosody and Jitsi Meet are added (GPG keys now in `files/`) - `ansible.builtin.service` => `ansible.builtin.systemd` - `ansible.builtin.shell` => `ansible.builtin.command`
mgauthier added 1 commit 2024-04-03 20:30:28 +02:00
Author
Owner

With this commit I changed...

  • one more ansible.builtin.shell => ansible.builtin.command
  • a few more jitsimeet_ prefix to variable names to avoid collision
With [this commit](https://gitea.evolix.org/evolix/ansible-roles/commit/2db3ed141458f20290c8d6ea22d44887338ddbf9) I changed... - one more `ansible.builtin.shell` => `ansible.builtin.command` - a few more `jitsimeet_ prefix` to variable names to avoid collision
mgauthier added 1 commit 2024-04-03 22:04:05 +02:00
Author
Owner

With this commit I added handlers.

With [this commit](https://gitea.evolix.org/evolix/ansible-roles/commit/5316373740158cd7b52d91ec4e15b658beb888b9) I added handlers.
mgauthier added 1 commit 2024-04-11 20:45:02 +02:00
mgauthier force-pushed jitsimeet from 0d9a15447a to 6f387b029c 2024-04-11 21:44:35 +02:00 Compare
This pull request is marked as a work in progress.
This branch is out-of-date with the base branch

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin jitsimeet:jitsimeet
git checkout jitsimeet

Merge

Merge the changes and update on Forgejo.
git checkout unstable
git merge --no-ff jitsimeet
git checkout unstable
git merge --ff-only jitsimeet
git checkout jitsimeet
git rebase unstable
git checkout unstable
git merge --no-ff jitsimeet
git checkout unstable
git merge --squash jitsimeet
git checkout unstable
git merge --ff-only jitsimeet
git checkout unstable
git merge jitsimeet
git push origin unstable
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: evolix/ansible-roles#176
No description provided.