Skip to content

feat/elasticsearch_watermarks#236

Merged
NavidSassan merged 9 commits into
mainfrom
feat/elasticsearch_watermarks
Apr 28, 2026
Merged

feat/elasticsearch_watermarks#236
NavidSassan merged 9 commits into
mainfrom
feat/elasticsearch_watermarks

Conversation

@ebuerki-lf

Copy link
Copy Markdown
Contributor

No description provided.

@ebuerki-lf
ebuerki-lf requested a review from NavidSassan April 28, 2026 08:16

@NavidSassan NavidSassan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

points from AI-review:

  1. CHANGELOG.md not updated (CONTRIBUTING.md:238). No ## [Unreleased] / ### Added entry for any of the five new variables.
  2. Inconsistent regex check between the four asserts (roles/elasticsearch/tasks/main.yml:143-178). Three asserts use the test form ... is
    ansible.builtin.regex("..."), but the fourth (high_max_headroom, line 174) uses the filter form | ansible.builtin.regex_search(...). They
    behave subtly differently (the filter returns the match or None; truthiness happens to work, but it's not the same as a boolean test). Make
    all four use is ansible.builtin.regex(...).
  3. Asserts are not in alphabetical order (CONTRIBUTING.md:76). Current order: flood_stage_frozen_max_headroom, flood_stage_max_headroom,
    low_max_headroom, high_max_headroom. Should be: …flood_stage_max_headroom, high_max_headroom, low_max_headroom. The defaults/main.yml and
    README ordering is already alphabetical — match it here too.
  4. Stray extra blank line between the low_max_headroom and high_max_headroom asserts (lines 169–170). Tasks inside a block use a single
    blank line; two blank lines are reserved for separating top-level - block:s (CONTRIBUTING.md:128).
  5. Missing range validation for the new percentage variable. elasticsearch__cluster_routing_allocation_disk_watermark_flood_stage_frozen is
    documented as Float (0 <= n <= 1), but unlike the existing three watermarks (asserts at lines 104–126), no 0 <= n <= 1 is number check was
    added. Add one in the same style.
  6. README example block missing the new variables (roles/elasticsearch/README.md:397-437).
  7. Regex case-sensitivity. ^[0-9]+(b|kb|mb|gb|tb|pb)$ rejects perfectly valid Elasticsearch values like 200MB, 1GB. Either add (?i) /
    IGNORECASE semantics, or document in the README that lowercase is required. The linked byte size units doc shows mixed-case values are
    accepted by ES.
  8. when: style. The folded > scalar form is unusual in this codebase — most when: clauses are either single-line strings or a list of
    strings (when: ['x is defined', 'x | length > 0']). The example role uses the list form (roles/example/tasks/main.yml:192-193). Switch for
    consistency.
  9. quiet: true on asserts (matches the example role pattern at roles/example/tasks/main.yml:54). The pre-existing asserts in this file also
    lack it, so this is a wider inconsistency, but worth fixing now for the new ones at minimum.
  10. Redundant is string guard in template. The four template if blocks gate on is defined and is string and (... | length) > 0. The is
    string part is redundant given the variables are documented as Type: String, validated by regex when set, and | length > 0 already trips on
    undefined-vs-empty. Drop is string.
  11. Conventional Commits not followed for any of the branch's commits (CONTRIBUTING.md:30-51):
  • fix typo in README
  • extend validation checks for _max_headroom variables
  • add link to elastic api conventions on byte size units
  • fix whitespace, quotation for cluster.routing.allocation.disk.watermark.*.max_headroom values
  • allow complete set of cluster.routing.allocation.disk.watermark.* to be set through elasticsearch.yml
  1. Duplicate flood_stage range assert at roles/elasticsearch/tasks/main.yml:120-134 — the same check exists twice (lines 120-126 and
    128-134, identical). Pre-dates this PR; could be cleaned up while you're touching this file.

Comment thread roles/elasticsearch/tasks/main.yml Outdated
Comment thread roles/elasticsearch/tasks/main.yml Outdated
Comment thread roles/elasticsearch/templates/etc/elasticsearch/elasticsearch.yml.j2 Outdated
Comment thread roles/elasticsearch/templates/etc/elasticsearch/elasticsearch.yml.j2 Outdated
Comment thread roles/elasticsearch/README.md
@NavidSassan
NavidSassan merged commit 217e941 into main Apr 28, 2026
5 checks passed
@NavidSassan
NavidSassan deleted the feat/elasticsearch_watermarks branch April 28, 2026 15:14
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