chore(ansible-lint.yml): remove yaml[line-length] from skip_list #417

ThomasSanson · 2023-07-14T14:05:23Z

This pull request introduces several enhancements to the PostgreSQL cluster configuration and testing. The changes are aimed at improving the readability, maintainability, and robustness of the codebase, as well as ensuring compatibility with different systems.

The yaml[line-length] rule was removed from the skip_list in ansible-lint configuration.

Improved Codebase: Several changes were made to improve the readability and maintainability of the codebase. This includes refactoring complex strings into variables, splitting URLs into parts, and improving variable and task naming for better clarity and understanding.
Enhanced Testing: New assert tests were added for pg_probackup_command_value and vip_manager_package_repo to ensure their correctness. Additional tests were added for wal_g_cron_jobs to validate its functionality. A new test file pg_probackup.yml was added to test the pg_probackup command. Tests for role swap in Ansible were also introduced, along with pre-checks for TimescaleDB.
Better Compatibility: The codebase was updated to ensure compatibility with RedHat systems. Debian specific tests were also added to ensure compatibility with Debian systems.

The yaml[line-length] rule was removed from the skip_list in ansible-lint configuration. This change was made to enforce the line length limit in the YAML files, improving readability and maintainability of the code.

Log files are typically not included in version control as they can be regenerated and can clutter the repository. Adding *.log to the .gitignore file ensures that these files are not accidentally committed.

…d vip_manager_package_repo 📝 docs(tests): add main.yml to include all assert tests in molecule tests The new assert tests for pg_probackup_command_value and vip_manager_package_repo are added to ensure the correctness of these values. This will help in maintaining the integrity of the application by catching any unexpected changes in these values. The main.yml file is added to include all assert tests in molecule tests, making it easier to manage and run all the tests from a single place.

…stgres The inclusion of the asserts playbook for postgres in the verify.yml file allows for more comprehensive testing. This change ensures that the postgres variables are correctly set and functioning as expected during the testing phase.

The new assertions are added to the molecule test to ensure the correctness of the wal_g_cron_jobs. This will help in validating the functionality of the wal_g_cron_jobs and ensure that it is working as expected. The assertions compare the actual job with the expected job and provide a success message if they match, and a failure message if they don't. This will make it easier to identify and fix any issues with the wal_g_cron_jobs.

…readability and maintainability The complex string used in the assertion of pg_probackup[0].value is now extracted into a separate variable named pg_probackup_first_value. This change improves the readability of the code and makes it easier to maintain, as any changes to the string will now only need to be made in one place.

…ommand 🔧 refactor(vars/main.yml): Replace hardcoded pg_probackup command with a variable The new test file pg_probackup.yml is added to test the pg_probackup command. This is to ensure that the command is correctly formed and works as expected. In vars/main.yml, the hardcoded pg_probackup command is replaced with a variable. This change makes the command more flexible and easier to manage. Now, the command can be easily updated or modified by just changing the variable value.

…euse existing command parts The changes were made to improve the readability and maintainability of the code. The variable name was changed from a generic "Setting host facts using complex arguments" to a more specific "Setting pg_probackup_first_value" to better reflect its purpose. Additionally, the command parts were reused to construct the 'pg_probackup_patroni_cluster_bootstrap_command' instead of repeating the same command, reducing redundancy and potential errors in the future.

…in_pg_probackup_command_value for better clarity The variable name 'pg_probackup_first_value' was not clear enough about its purpose. Renaming it to 'origin_pg_probackup_command_value' provides a better understanding of its role in the code, which is to hold the original command value for pg_probackup. This change improves code readability and maintainability.

vars/main.yml

…ent command concatenation A comment has been added to the pg_probackup_command_parts variable to ensure that there is a space at the beginning of each part. This is to prevent the commands from concatenating, which could lead to unexpected behavior or errors. This change improves the readability and maintainability of the code.

…ter readability The comment line for the 'postgresql_restore_command' was too long and was split into multiple lines to improve readability. This makes it easier to understand the command and its parameters.

…sql_restore_command The trailing spaces in the commented out postgresql_restore_command were removed to improve code readability and maintain a clean codebase.

…st_job to origin_wal_g_cron_jobs_create_job and move job parts to vars/main.yml The renaming of wal_g_cron_jobs_first_job to origin_wal_g_cron_jobs_create_job provides a more descriptive name, making it easier to understand its purpose. The job parts of wal_g_cron_jobs are moved to vars/main.yml to improve modularity and reusability. This allows the job parts to be used in multiple places without duplication, making the code more maintainable.

…n_jobs[1].job 🔧 refactor(main.yml): add wal_g_cron_delete_job_parts and refactor job assignment The job naming in wal_g_cron_jobs.yml has been improved for better readability and understanding of the code. A new job, wal_g_cron_jobs[1].job, has been added to handle the deletion of old backups, enhancing the backup management process. In main.yml, a new variable, wal_g_cron_delete_job_parts, has been introduced to store the command for the deletion job. This makes the command reusable and the code cleaner. The assignment of the job command has also been refactored to use this new variable, improving the maintainability of the code.

…h job part 🔧 chore(main.yml): add a new job part in wal_g_cron_create_job_parts A comment has been added to remind developers to ensure there is a space at the beginning of each job part to prevent commands from concatenating. This is important to maintain the correct syntax and prevent errors during execution. Additionally, a new job part has been added to the wal_g_cron_create_job_parts variable. This new part checks the HTTP status code of the Patroni REST API and logs the output of the backup-push command to a file, which will help in debugging and monitoring the backup process.

…rs and refactor package repo URL 🔧 fix(vip_manager_package_repo): update assert to compare with origin_vip_manager_package_repo The changes were made to switch from Debian to RedHat variables in the Ansible playbook. This is to ensure compatibility with RedHat systems. The vip_manager_package_repo URL was also refactored into parts and joined together for better readability and maintainability. The assert was updated to compare the vip_manager_package_repo with the origin_vip_manager_package_repo to ensure the correct URL is being used.

The newline at the end of the file was added to comply with the POSIX standard, which defines a line as a sequence of zero or more non-newline characters followed by a newline character. This helps to avoid potential issues with programs that may not correctly handle or interpret the last line if it doesn't end with a newline.

…nes into multiple lines The comments in the RedHat.yml file were too long and hard to read. By splitting them into multiple lines, the readability of the comments is improved, making it easier for other developers to understand the code.

… readability and maintainability 🔧 refactor(tests): add Debian specific tests to ensure compatibility The URL for the vip_manager_package_repo in Debian.yml has been split into parts to improve readability and maintainability. This makes it easier to understand and modify individual parts of the URL. Additionally, Debian specific tests have been added to the vip_manager_package_repo.yml file to ensure that the application is compatible with Debian systems. This will help catch any Debian-specific issues early in the development process.

…h rule for ansible.builtin.set_fact The yamllint line-length rule was disabled for ansible.builtin.set_fact to avoid linting errors due to the long URL. This change improves readability and maintainability of the code by preventing unnecessary line breaks in the URL.

📝 docs(molecule): add comments to explain the purpose of each test The changes include the addition of tests for the role swap in Ansible. This is important to ensure that the role swap functionality is working as expected. The tests include conditions for creating and deleting swap. Comments have been added to each test to explain what each test is checking for, making it easier for other developers to understand the purpose of each test.

…ml and main.yml 📝 docs(swap): add descriptive name to main.yml for better context The condition checks in create.yml and main.yml have been refactored to span multiple lines for better readability and maintainability. This makes it easier to understand the logic behind the conditions. Additionally, a descriptive name has been added to main.yml to provide better context when running the tests.

…tanding The task names have been updated to provide more context and clarity. The task previously named "Molecule | Tests | Include all asserts" is now named "Molecule | Tests | Variables | Include all tests". This change makes it easier to understand the purpose of the task and what it is expected to do.

🔧 refactor(timescaledb.yml): improve readability of shared_preload_libraries_item variable assignment The pre-checks role and its corresponding tests have been added to ensure that 'timescaledb' is included in 'shared_preload_libraries'. This is crucial for the proper functioning of TimescaleDB. The shared_preload_libraries_item variable assignment in timescaledb.yml has been refactored for better readability and maintainability.

…pre-checks This new test file calculates the total pool size for PgBouncer, a PostgreSQL connection pooler, and validates it against a predefined expected value. This is to ensure that the total pool size does not exceed the maximum allowed connections. The test data includes two databases with defined pool sizes. The calculated total pool size is then compared with the expected value, and the test passes if they match. This addition enhances the robustness of our pre-checks by ensuring the pool size is correctly configured before deployment.

…ncer_pool_size for clarity 📝 docs(pgbouncer.yml): update comments to reflect changes and improve readability The variable name 'origin_pgbouncer_pool_size' was changed to 'pgbouncer_pool_size' to make it more intuitive and clear. The comments were also updated to better explain the purpose of the code and the calculations being performed. This makes the code easier to understand and maintain.

… calculation 🔨 feat(pgbouncer.yml): add loop_control for better loop variable naming The calculation of pgbouncer_pool_size was refactored to improve readability and maintainability. The calculation is now broken down into multiple lines, making it easier to understand each step. Additionally, loop_control was added to the loop to provide a more descriptive variable name (pool_item) for each iteration, improving code clarity.

….yml 🔍 test(tests): add new tests to verify total pool size calculation in pgbouncer.yml The changes in the pgbouncer.yml file are aimed at improving readability and clarity of the test descriptions. The comments have been rewritten to be more descriptive and clear. The task names have been updated to follow a more structured and detailed naming convention. New tests have been added to verify the total pool size calculation across all databases. These tests ensure that the pool size calculation is accurate even when some databases do not have a corresponding pool defined in 'pgbouncer_pools'. This increases the robustness of the tests and ensures that the pool size calculation logic works as expected in all scenarios.

roles/patroni/tasks/custom_wal_dir.yml

…i_config variables for better readability and maintainability The changes were made to improve the readability and maintainability of the code. By replacing the generic "item" variable with specific "patroni_config" variables, it becomes clearer what each part of the code is doing. This makes it easier for other developers to understand and modify the code in the future.

The maximum line length in the .yamllint configuration file has been set to 160 characters. This change is made to enforce a consistent code style across the project and to improve readability by preventing excessively long lines of code.

…em naming The loop_var has been added to the loop_control in the main.yml file to improve the naming of the loop items. This change makes the code more readable and easier to understand, as the loop items now have more descriptive names: 'patroni_config_without_cluster_vip_item' and 'patoni_config_with_cluster_vip_item'. This is particularly useful for debugging and maintaining the code.

molecule/tests/roles/confd/variables/haproxy.tmpl.yml

molecule/tests/roles/haproxy/variables/haproxy.cfg.yml

molecule/tests/roles/patroni/variables/custom_wal_dir.yml

…nctionality The task description was previously misleading as it mentioned replacing "bind" for stats, which was not accurate. The task actually replaces any line that starts with "bind", regardless of whether it's for stats or not. The updated description now accurately reflects this functionality.

…y.cfg template file The task name was simplified to improve readability and clarity. The specific mention of 'stats' was removed as it was not necessary for understanding the purpose of the task, which is to prepare the haproxy.cfg template file by replacing lines that start with 'bind'.

The task names in the haproxy.tmpl.yml file were updated to improve clarity and readability. The phrase "for stats" was removed from the task names as it was not necessary and could potentially cause confusion. The updated task names now accurately reflect the actions being performed.

…in haproxy.tmpl and haproxy.cfg The port numbers were changed to avoid conflicts with other services that might be running on the same ports. The new port numbers (5000-5004) are less commonly used, reducing the likelihood of port conflicts. The corresponding test assertions were also updated to reflect these changes.

… test scenario 2 The postgresql_version for the second test scenario in custom_wal_dir.yml was updated from 9 to 9.6. This change was necessary to ensure that the tests are running against the correct version of PostgreSQL, which is 9.6 in this case.

vitabaks · 2023-07-16T19:58:38Z

@ThomasSanson

First and foremost, I would like to express my gratitude for the effort you have put into improving and expanding our Molecule test suite. I am confident that these enhancements will significantly boost the reliability and resilience of our project.

You've truly done an immense job adding tests in places where many of us wouldn't have thought to put them. This undoubtedly helps enhance the quality of our code and makes it more resilient to potential bugs.

However, I would like to kindly ask you to split this pull request into two separate ones: one for the 'line-length' related changes and another for the new tests you've added. Separating these changes into two distinct pull requests would allow us better track and document these modifications in future release notes.

As a rule, tests and code changes are treated as separate groups when compiling release notes. Hence, it would be great if you could provide them in two distinct pull requests.

vars/main.yml

ThomasSanson · 2023-07-16T21:07:11Z

@ThomasSanson

First and foremost, I would like to express my gratitude for the effort you have put into improving and expanding our Molecule test suite. I am confident that these enhancements will significantly boost the reliability and resilience of our project.

You've truly done an immense job adding tests in places where many of us wouldn't have thought to put them. This undoubtedly helps enhance the quality of our code and makes it more resilient to potential bugs.

However, I would like to kindly ask you to split this pull request into two separate ones: one for the 'line-length' related changes and another for the new tests you've added. Separating these changes into two distinct pull requests would allow us better track and document these modifications in future release notes.

As a rule, tests and code changes are treated as separate groups when compiling release notes. Hence, it would be great if you could provide them in two distinct pull requests.

Dear @vitabaks,

Firstly, I am heartened by your acknowledgement and kind words regarding the efforts I've put into improving our Molecule test suite. It is indeed a source of personal satisfaction to know that my contributions have been well received and deemed valuable

Whilst I fully appreciate your request to split the current pull request into two distinct ones, I'm afraid I must respectfully disagree on this occasion. In this specific context, the additions to the test suite are tightly coupled to the changes made in the code

The code modifications and corresponding tests aren't isolated; rather, they function in unison to guarantee the correct performance of the updated feature. To be more precise, each test is directly linked to the change brought about by the removal of the 'line-length' rule

Therefore, separating them could potentially obscure this fundamental association, and could inadvertently lead to potential mishaps in the future. Besides, you may have noticed that the tests fail when an incorrect formatting is introduced

This isn't to be mistaken for a disregard towards the practice of adding global tests incrementally. I completely endorse this practice and assure you that separate pull requests would be made for scenario-based end-to-end tests. However, in this case, the tests added are specific to the changes introduced and are conceived in a Test-Driven Development (TDD) manner

I believe this might explain the current confusion regarding the 'pgbouncer_pool_size' test, among others. Rest assured, the tests introduced now provide a solid foundation and have room for evolution in line with future modifications

Once again, I truly appreciate your insights and constructive feedback. It is this spirit of collaboration and mutual respect that fosters a dynamic and efficient working environment

… mv_command_postgresql_version_9 assertion The PostgreSQL version in the mv_command_postgresql_version_9 assertion was updated from 9 to 9.6 to reflect the correct version being used. This ensures that the test assertion is accurate and will pass when the correct version of PostgreSQL is in use.

…tore_command for better clarity The variable name 'pg_probackup_command' was not clear enough about its purpose. Renaming it to 'pg_probackup_restore_command' provides a better understanding of its role in the code, which is to restore the PostgreSQL backup.

molecule/tests/roles/haproxy/variables/haproxy.cfg.yml

…t numbers for better organization and performance The port numbers for different services in the haproxy configuration have been reordered and updated. The 'stats' port has been moved to 7000 to separate it from the other services and to avoid potential conflicts. The other services' ports have been renumbered starting from 5000 for better organization and to improve the performance of the system. The corresponding tests have also been updated to reflect these changes.

vars/main.yml

…ob_parts to wal_g_backup_command for clarity The variable wal_g_cron_create_job_parts was renamed to wal_g_backup_command to better reflect its purpose. This change improves code readability and understanding, as the new name clearly indicates that this variable holds the command for creating a backup.

…mand' for better clarity The variable 'wal_g_cron_delete_job_parts' was renamed to 'wal_g_delete_command' to better reflect its purpose. The old name was misleading as it suggested that the variable was part of a cron job, while in reality, it is a command used to delete old backups. The new name is more intuitive and accurately describes the variable's function.

ThomasSanson added 2 commits July 14, 2023 10:32

🔧 chore(ansible-lint.yml): remove yaml[line-length] from skip_list

057df41

The yaml[line-length] rule was removed from the skip_list in ansible-lint configuration. This change was made to enforce the line length limit in the YAML files, improving readability and maintainability of the code.

🔧 chore(.gitignore): add *.log to ignore log files

916f089

Log files are typically not included in version control as they can be regenerated and can clutter the repository. Adding *.log to the .gitignore file ensures that these files are not accidentally committed.

ThomasSanson force-pushed the yaml-line-length branch from b4abf65 to 916f089 Compare July 14, 2023 15:02

ThomasSanson added 7 commits July 14, 2023 15:03

ThomasSanson commented Jul 14, 2023

View reviewed changes

vars/main.yml Show resolved Hide resolved

ThomasSanson added 19 commits July 14, 2023 16:16

📝 docs(main.yml): split long comment line into multiple lines for bet…

958486c

…ter readability The comment line for the 'postgresql_restore_command' was too long and was split into multiple lines to improve readability. This makes it easier to understand the command and its parameters.

🔧 refactor(main.yml): remove trailing spaces in commented out postgre…

9685bdb

…sql_restore_command The trailing spaces in the commented out postgresql_restore_command were removed to improve code readability and maintain a clean codebase.

ThomasSanson commented Jul 16, 2023

View reviewed changes

roles/patroni/tasks/custom_wal_dir.yml Show resolved Hide resolved

ThomasSanson marked this pull request as ready for review July 16, 2023 15:04

ThomasSanson requested a review from vitabaks July 16, 2023 15:04

ThomasSanson added 2 commits July 16, 2023 15:06