Enable deployment of `engine` and `federation-api` on a single server #41

Ndpnt · 2024-05-20T09:21:36Z

No description provided.

clementbiron

These are great changes 👏 and we should to spend some time together so that I can become a little more independent with the deployment.

roles/github/README.md

playbooks/deploy.yml

README.md

MattiSG

Really good changelog, congrats!!

I am surprised that we trigger installs (of pm2, of NGINX…) conditionally based on a variable passed. I find it confusing that the playbook calls the same roles twice with a install: false variable the second time. I understand that a configuration management system should always define the dependencies to be installed, and that it is up to the system to assess whether the install is necessary or not. Otherwise, I wonder if splitting the install and management in separate (admittedly tiny) roles would not be appropriate.

I tried to check locally with Vagrant, but I always get a 502 when trying to connect to http://localhost:8080/collection-api/v1/docs/ or any other variation. Are the forwarded ports up to date? If so, describing in the “Testing” section which URL to access would be useful.

I would personally welcome an Ansible provisioning in the Vagrantfile to speed up the testing process 🙂

CHANGELOG.md

README.md

tests/Vagrantfile

MattiSG · 2024-05-28T11:51:41Z

tests/inventory.yml

+      ansible_ssh_host: 127.0.0.1
+
+      ota_source_repository: https://github.com/OpenTermsArchive/demo-declarations.git
+      ota_source_repository_branch: test-new-config


This should be updated before merge.

tests/pm2.config.cjs

MattiSG · 2024-05-28T11:54:21Z

tests/pm2.config.cjs

+      args: 'run start:federation-api',
+      max_restarts: 2,
+      min_uptime: '1h', // Set a relatively high duration (more than the longest run) so that restarts that occur before this duration has elapsed are considered unstable.
+      restart_delay: 15 * 60 * 1000,


This value seems too low for that app that reusers might depend on.

Ndpnt · 2024-05-28T13:14:27Z

I would personally welcome an Ansible provisioning in the Vagrantfile to speed up the testing process 🙂

I considered it, but as a tester, I prefer to have full control over the playbook execution and maintain a clear separation between Vagrant VM initialization and playbook execution.
Sometimes, I just want to initialize the VM, connect to it, check some things, and only then execute the playbook. Additionally, having everything defined in the Vagrantfile does not allow for refined execution with tags.
Finally, I'd prefer the playbook execution in test mode to be as close as possible to what I might need to do if I had to manipulate the production server with the playbook. This approach helps me become and remain familiar with the ansible-playbook command.
And when I need to repeat the clean setup multiple times, I simply run the following commands vagrant destroy -f && vagrant up && ansible-playbook ../playbooks/deploy.yml.

MattiSG

The README is really neat and readable with this file-based approach, love it!

And the infrastructure setup code is just… beautiful 🥲

Really good results after this pass of polish, congratulations!

MattiSG · 2024-05-31T08:01:37Z

README.md

-These variables can be defined in the inventory file, for example:
+The `inventory.yml` file defines the hosts and the variables required for the deployment. This file should contain all the necessary variables as described below.
+
+| Variable                       | Description                                                                                                                | Required or default Value |


MattiSG · 2024-05-31T08:04:17Z

README.md

-      ota_source_repository_branch: main
-      ota_directory: demo
+      ota_source_repository_branch: master
+      ota_directory: opentermsarchive-demo


In this case, wouldn't “demo” be a valid example? I understand that the “name of the repository” default would be demo-declarations and not demo, and that it would thus be relevant to set ota_directory: demo.

MattiSG · 2024-05-31T08:19:48Z

playbooks/deploy.yml

+        apply:
+          become: true


Why do we apply the become here rather than become in the role itself? Is there a way in which mongo/configure can run without become?

I decided to manage all become: true settings at the playbook level for the following reasons:

Centralization of privilege escalation in one place.

Setting become: true once at the playbook level simplifies the roles, avoiding repetitive configuration in each task.

However, this approach has some drawbacks:

It might escalate privileges for tasks that don't require it, but this it seems to me that it is not an issue in our case as each task in infrastructure need privileges.

It reduces the control over which specific tasks need privilege escalation. However, the way our roles are structured means this granularity is not necessary.

Another solution considered was using block in roles to wrap all tasks with a become: true instruction. However, similar to defining global variables, I believe it is clearer to specify where privilege escalation will occur at the entry point of the playbook.

Amazing, thank you for these clear explanations!

MattiSG · 2024-05-31T08:21:04Z

playbooks/deploy.yml

-      - name: Install Node
-        ansible.builtin.include_role:
-          name: node
+    - name: Set required variables


Extracting this into a dedicated role would make deploy even more readable 🙂

I don't think so, I prefer to have a top down approach for global variables

MattiSG · 2024-05-31T08:23:32Z

roles/mongo/configure/tasks/main.yml

+---
+- name: Configure MongoDB
+  notify: Restart MongoDB
+  block:


Why is this all in a block instead of having 3 steps?

For the notify

MattiSG · 2024-05-31T08:26:12Z

roles/nginx/install/tasks/main.yml

@@ -0,0 +1,6 @@
+---
+- name: Install NGINX package


Considering how small that role is, I would also have been fine seeing it straight in the playbook 🙂

tests/pm2.config.cjs

README.md

Co-authored-by: Clément Biron <[email protected]>

Co-authored-by: Matti Schneider <[email protected]>

Ndpnt force-pushed the v2 branch 2 times, most recently from b4c32ae to b35766e Compare May 20, 2024 09:24

Ndpnt requested review from MattiSG and clementbiron May 22, 2024 07:03

clementbiron approved these changes May 22, 2024

View reviewed changes

MattiSG requested changes May 28, 2024

View reviewed changes

Ndpnt requested a review from MattiSG May 29, 2024 15:17

Ndpnt added 4 commits May 31, 2024 14:09

Flatten roles structure

86f77fc

Extract github role

fb35d57

Create pm2 role

ef2cd8c

Rewrite Nginx role

dd73717

MattiSG approved these changes May 31, 2024

View reviewed changes

Ndpnt added 17 commits May 31, 2024 14:11

Rewrite how OTA applications are deployed

4ada481

Update tests

aabe4d5

Expose VM remote port 80 to host 8080 to test APIs

1d0c9c5

Rename ota/applications in ota/apps

602986b

Remove obsolete file

211eccb

Improve variable name

e6d1e06

Rename federated-api in federation-api

7bc6266

Ensure ota apps variables are available to the play

a92374a

Remove obsolete code

0b3f291

Name the VM

398d620

Avoid to specify to ansible the python interpreter

a0bd647

Update test inventory

a417c67

Improve variables naming

a69744b

Fix .env file existence check on local machine

a136773

Add Github private file existence check on local machine

60a1695

Make pm2 config file required next to the inventory

13511cf

Ensure delegate_to works with Vagrant

0601316

Ndpnt and others added 24 commits May 31, 2024 14:11

Add changelog entry

ceb81be

Improve copywriting

c360ddd

Co-authored-by: Clément Biron <[email protected]>

Fix application name

2d215fe

Co-authored-by: Clément Biron <[email protected]>

Display all variables in example

79ecd59

Co-authored-by: Clément Biron <[email protected]>

Remove useless READMEs

71cba8a

Lint

55b23b0

Improve wording

b6f623c

Co-authored-by: Matti Schneider <[email protected]>

Improve documentation

ad5a05e

Co-authored-by: Matti Schneider <[email protected]>

Improve roles structure

f857f5c

Use long option name

e8eb61d

Improve mongo role structure

975d50a

Make block more readable

54b60b4

Improve deploy playbook

5b1fafe

Rename role

a23df23

Remove obsolete condition

37568e7

Update test pm2 config

36604f6

Fix deploy playbook

ee875e0

Factorize condition

faab2b7

Group entries in changelog

118622a

Update Readme

08592ec

Avoid regex

afccdf0

Fix variable definition

cf20df2

Improve git based database role

6d80206

Use ota_apps_config variables only in the playbook

407b470

Ndpnt force-pushed the v2 branch from 9c0307b to 407b470 Compare May 31, 2024 12:11

Ndpnt and others added 3 commits May 31, 2024 14:33

Improve restart delays

634498f

Co-authored-by: Matti Schneider <[email protected]>

Minor README improvements

a13f2d5

Remove obsolete test config

c3200a0

Ndpnt merged commit ef9a99e into main May 31, 2024
1 check passed

Ndpnt deleted the v2 branch May 31, 2024 12:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable deployment of `engine` and `federation-api` on a single server #41

Enable deployment of `engine` and `federation-api` on a single server #41

Ndpnt commented May 20, 2024

clementbiron left a comment

MattiSG left a comment

MattiSG May 28, 2024

MattiSG May 28, 2024

Ndpnt commented May 28, 2024

MattiSG left a comment

MattiSG May 31, 2024

MattiSG May 31, 2024

MattiSG May 31, 2024

Ndpnt May 31, 2024

MattiSG May 31, 2024

MattiSG May 31, 2024

Ndpnt May 31, 2024

MattiSG May 31, 2024

Ndpnt May 31, 2024

MattiSG May 31, 2024

	\| Variable \| Description \| Required or default Value \|
	\| Variable \| Description \| Required or default value \|

Enable deployment of engine and federation-api on a single server #41

Enable deployment of engine and federation-api on a single server #41

Conversation

Ndpnt commented May 20, 2024

clementbiron left a comment

Choose a reason for hiding this comment

MattiSG left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ndpnt commented May 28, 2024

MattiSG left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Enable deployment of `engine` and `federation-api` on a single server #41

Enable deployment of `engine` and `federation-api` on a single server #41