vitabaks / postgresql_cluster

PostgreSQL High-Availability Cluster (based on Patroni). Automating with Ansible.
https://postgresql-cluster.org
MIT License
1.69k stars 411 forks source link

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

Closed ThomasSanson closed 1 year ago

ThomasSanson commented 1 year ago

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.

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

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

  3. Better Compatibility: The codebase was updated to ensure compatibility with RedHat systems. Debian specific tests were also added to ensure compatibility with Debian systems.

ThomasSanson commented 1 year ago

@vitabaks

giphy (51)

🤣

vitabaks commented 1 year ago

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

ThomasSanson commented 1 year ago

@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