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 buster #1

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

Install buster #1

wants to merge 21 commits into from

Conversation

tricovictor
Copy link
Member

Parameterize the code and adapt it to Buster

tasks/install.yml Outdated Show resolved Hide resolved
tasks/postgres.yml Outdated Show resolved Hide resolved
tasks/configuration.yml Outdated Show resolved Hide resolved
tasks/configuration.yml Outdated Show resolved Hide resolved
tasks/configuration.yml Outdated Show resolved Hide resolved
defaults/main.yml Outdated Show resolved Hide resolved
@@ -5,7 +5,94 @@ peertube_version: v1.0.0-beta.3
peertube_user_path: /var/www/peertube
peertube_proxy_handle_https: no
peertube_proxy_ips: []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsed variable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

peertube_user_path aquí
peertube_proxy_handle_https aquí
peertube_proxy_ips now changes peertube_trust_proxy aquí with default loopback value.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is necessary the name changing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the name is defined like this in the configuration file here.

# If you run PeerTube just behind a local proxy (nginx), keep 'loopback'
# If you run PeerTube behind a remote proxy, add the proxy IP address (or subnet)
trust_proxy:
- 'loopback'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...peertube_proxy_ips can be used here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

peertube_proxy_ips now peertube_trust_proxy is used here

defaults/main.yml Outdated Show resolved Hide resolved
templates/enabledLDAP.sql Outdated Show resolved Hide resolved
Copy link
Member

@andrespias andrespias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 new comments

when:
- peertube_version_dir.stat.exists == False
- fresh_install # is changed
- peertube_web_admin_password is defined
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

peertube_web_admin_password is also unused any more. Should this code be deleted for all cases?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected. Used here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Genial! Mirando el rol original deberíamos hacer un mayor uso de la variable fresh_install para las tareas subsiguientes. Por ejemplo, ejecutar las SQL extra queries solo si estamos en una fresh_install no te parece?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that if we change something in the ldap, for example, we need the sql statement to be executed again for the changes to be reflected in the system.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine in this case, but to keep the logic of the original role we can add a when: fresh_install is changed here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added here

notify:
- install peertube daemon

- name: Add tools in $PATH
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why was this deleted?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

npm is used to install the plugins and not cli wraper

tasks/install.yml Outdated Show resolved Hide resolved
tasks/main.yml Outdated Show resolved Hide resolved
tasks/configuration.yml Outdated Show resolved Hide resolved
tasks/configuration.yml Outdated Show resolved Hide resolved
@andrespias
Copy link
Member

In this branch we also have to remove this unused dependency (meta/main.yml):
- kwoodson.yedit

tasks/postgres.yml Outdated Show resolved Hide resolved
tasks/postgres.yml Outdated Show resolved Hide resolved
tasks/postgres.yml Outdated Show resolved Hide resolved
tasks/postgres.yml Outdated Show resolved Hide resolved
tasks/postgres.yml Outdated Show resolved Hide resolved
Copy link
Member

@andrespias andrespias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check the use of peertube_user/peertube_postgres_user variables.

@@ -96,20 +96,21 @@
template:
src: sql/{{ item }}
dest: /tmp/{{ item }}
owner: postgres
owner: '{{ peertube_user }}'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is {{ peertube_postgres_user }}, does it ?? and wihout group..

tasks/postgres.yml Outdated Show resolved Hide resolved
tasks/postgres.yml Outdated Show resolved Hide resolved
tasks/postgres.yml Outdated Show resolved Hide resolved
@andrespias
Copy link
Member

When i run the role with default vars (no value for peertube_alias) i get this:

TASK [udelarinterior.peertube : Copy nginx conf file] ***********************************************************************************************************
task path: /home/apias/desarrolloudelar/ansible/UdelaRInterior/udelarinterior.peertube/tasks/nginx.yml:2
fatal: [pericon.interior.edu.uy]: FAILED! => {"changed": false, "msg": "AnsibleUndefinedVariable: 'peertube_alias' is undefined"}

@andrespias
Copy link
Member

...It's time to update README.md also.

@@ -1,11 +1,136 @@
---
# defaults file for peertube
peertube_tld: localhost
peertube_tld: '{{ inventory_hostname }}'
#peertube_alias: []
Copy link
Member

@andrespias andrespias Jun 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default value must be ["{{ inventory_hostname }}"]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default aliases are not defined, if you have one we do it in the vars of the container.

hd: false #1080p
full_hd: false #2160p

peertube_plugins:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default value should be [] otherwise the role fails with this:

TASK [udelarinterior.peertube : Install plugins npm package] ****************************************************************************************************
task path: /home/apias/desarrolloudelar/ansible/UdelaRInterior/udelarinterior.peertube/tasks/install_configure_peertube.yml:91
fatal: [pericon.interior.edu.uy]: FAILED! => {"msg": "Invalid data passed to 'loop', it requires a list, got this instead: None. Hint: If you passed a list/dict of just one element, try adding wantlist=True to your lookup invocation or use q/query instead of lookup."}

Copy link
Member Author

@tricovictor tricovictor Jun 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added here

Copy link
Member

@andrespias andrespias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More comments after running the role with default values

- fresh_install # is changed
- peertube_web_admin_password is defined
- include_tasks: copy_ldap_key_tls.yml
when: not peertube_ldap.insecure_tls
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also check that peertube_ldap is defined. Now the role fails if it's undefined:

TASK [udelarinterior.peertube : include_tasks] ******************************************************************************************************************
task path: /home/apias/desarrolloudelar/ansible/UdelaRInterior/udelarinterior.peertube/tasks/main.yml:12
fatal: [pericon.interior.edu.uy]: FAILED! => {"msg": "The conditional check 'not peertube_ldap.insecure_tls' failed. The error was: error while evaluating conditional (not peertube_ldap.insecure_tls): 'None' has no attribute 'insecure_tls'\n\nThe error appears to be in '/home/apias/desarrolloudelar/ansible/UdelaRInterior/udelarinterior.peertube/tasks/main.yml': line 12, column 3, but may\nbe elsewhere in the file depending on the exact syntax problem.\n\nThe offending line appears to be:\n\n\n- include_tasks: copy_ldap_key_tls.yml\n  ^ here\n"}


Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definded here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running with new changes (peertube_ldap:[] in defaults) I get the same error:

TASK [udelarinterior.peertube : include_tasks] ******************************************************************************************************************
task path: /home/apias/desarrolloudelar/ansible/UdelaRInterior/udelarinterior.peertube/tasks/main.yml:12
fatal: [pericon.interior.edu.uy]: FAILED! => {"msg": "The conditional check 'not peertube_ldap.insecure_tls' failed. The error was: error while evaluating conditional (not peertube_ldap.insecure_tls): 'list object' has no attribute 'insecure_tls'\n\nThe error appears to be in '/home/apias/desarrolloudelar/ansible/UdelaRInterior/udelarinterior.peertube/tasks/main.yml': line 12, column 3, but may\nbe elsewhere in the file depending on the exact syntax problem.\n\nThe offending line appears to be:\n\n\n- include_tasks: copy_ldap_key_tls.yml\n  ^ here\n"}

We will have to use is defined condition as we talk about it.

loop: "{{ peertube_sql_extras }}"
notify: restart peertube daemon

- name: Update dependencies
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To keep the logic of the original role we can add a when: fresh_install is changed here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added here

tasks/main.yml Outdated
update_cache: yes
pkg: nodejs
state: latest
- include_tasks: nginx.yml
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First certbot.yml, then nginx.yml, otherwise Nginx cannot read ssl certificates generated

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can't because the certbot task adds the location of the certificate to the nginx configuration. If we install cerbot first, it won't find the nginx config file. Here

dest: /etc/nginx/sites-available/peertube
marker: " # {mark} let's encrypt configuration"
block: |2-
ssl_certificate /etc/letsencrypt/live/{{ peertube_tld }}/cert.pem;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We must to use fullchain.pem por federeation instead of cert.pem

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