2 commits

Author	SHA1	Message	Date
Eva Sarafianou	5cacd26776	Fix config-change-checker to use merge-base for per-file diffs (#36670) ... Automatic Merge	2026-05-21 10:23:44 +02:00
Amy Blais	0675d0ea0b	Automations for config.json, API, audit log event, and Go release notes (#36075) ... * Create config-change-checker.yml * Create check_config_changes_ci.py * Update config-change-checker.yml * Update check_config_changes_ci.py * Update check_config_changes_ci.py * Update check_config_changes_ci.py * Update check_config_changes_ci.py * Update config-change-checker.yml * Update check_config_changes_ci.py * Update config-change-checker.yml * Update config.go * Fix check_api to detect multi-line and multi-method endpoints The previous implementation matched the .Handle(...).Methods(...) regex line-by-line against diff lines. This silently missed two real and common patterns in api4/: 1. Multi-line .Handle(...) declarations — e.g. group.go has 18 of them, where the path lives on one line and the wrapper/handler on the next. The regex never matched, so PRs adding such endpoints produced empty release-note entries. 2. Multi-method declarations like .Methods(http.MethodGet, http.MethodHead) (4 instances in file.go) — the old regex required a closing paren immediately after the first method. The fix: - Add a file_at(ref, path) helper that snapshots a file at a git ref via 'git show', so checkers can compare full file states instead of pattern-matching diff text. - Add _scan_endpoints() that whitespace-collapses the file before matching, letting the regex span what were originally multiple lines. - Loosen _HANDLE_RE to capture the methods list as a substring and extract individual HTTP verbs with a known-method allowlist, so multi-method declarations produce one entry per verb. - Switch check_api to set-diff (after - before) / (before - after) on the parsed endpoint sets. This also cleanly handles routes that move within a file (no fragile add/remove dedup needed). - Anchor the new/deleted file detection to '^new file mode \d+' to avoid false positives from stray text in source files. Made-with: Cursor * Track enclosing struct in check_config to avoid dedup collisions The previous check_config keyed its add/remove dedup on the bare field name. The dedup intent was to ignore fields that were merely reordered within config.go (which appear in the diff as both '-Foo' and '+Foo'). But because the key was just the field name, an unrelated rename in one struct could silently cancel out a real new field with the same name in a different struct. For example, in a single PR: - EnableFoo *bool // removed from ServiceSettings + EnableFooV2 *bool - EnableBar *bool // removed from EmailSettings + EnableFoo *bool // newly added — but wrongly cancelled below The dedup would see 'EnableFoo' in both lists and drop both entries, hiding the brand-new EmailSettings.EnableFoo from the release-note output. The fix tracks each field's enclosing struct using a brace-depth stack that walks the file at BASE_SHA and HEAD_SHA. Fields are keyed as (struct_name, field_name) tuples, so identically-named fields in different structs are distinct, and the dedup only collapses true reorderings. As a side benefit the rendered output is now 'StructName.FieldName' which is much more useful to reviewers. Switching to file-at-revision scanning + set diff also removes the custom dedup logic entirely — set arithmetic handles "moved within file" naturally. Made-with: Cursor * Switch remaining checkers to file-at-revision style; drop lines_by_sign check_audit_events and check_go_version still parsed +/- diff lines directly, with the same brittle dedup-and-cancel logic that was used in the previous check_config. After the previous two commits the rest of the file uses the file_at(ref, path) helper to compare full file states between BASE_SHA and HEAD_SHA, which: - removes the entire moved-within-file dedup dance (set arithmetic handles it for free), - aligns all four checkers on a single, easy-to-reason-about pattern, - is robust to whitespace-only or reordering edits in the watched files. For Dockerfile.buildenv the helper also avoids a subtle case where the old code only inspected +/- lines: an edit to an unrelated RUN line that didn't touch the FROM line could in theory leave both old_ver and new_ver as None even though the version was effectively unchanged. Reading the file at each revision compares the actual current and previous FROM line directly. The lines_by_sign helper now has no callers, so remove it. Made-with: Cursor * Update config.go * Update config.go * Update check_config_changes_ci.py * Update check_config_changes_ci.py * Update check_config_changes_ci.py * Update check_config_changes_ci.py * Tighten check_config_changes_ci.py: regex coverage + idempotency - Restore tolerant `_HANDLE_RE` so 2-arg wrappers (e.g. `api.APISessionRequired(handler, handlerParamFileAPI)`) are not silently dropped from the api4 endpoint scan; broaden the `.Methods(...)` capture so string-literal variants (`Methods("GET")`) work too. Filtering moves back to the `_HTTP_METHODS` allowlist in `_parse_methods` to keep stray identifiers from being treated as HTTP verbs. - Make `strip_old_note` also remove auto-generated lines that landed outside the ```release-note fence (the inject_note fallback paths) so reruns no longer accumulate duplicates when a PR has no fence. - Skip the GitHub PATCH when the PR description is already up to date, so every commit no longer triggers an unconditional write. - Wire up `check_go_version`'s `additions` path in `_format_lines` and `_AUTO_LINE_RE` so a freshly-added Dockerfile.buildenv emits a note. - Remove the now-dead `CheckResult.to_markdown` method (replaced by `_format_lines`). Made-with: Cursor * Restore ExperimentalSettings.EnableWatermark The field was removed in f71527f0b1 but `server/config/client.go`, `server/config/client_test.go`, and `server/public/model/config_test.go` still reference it (added on master in #36025). Restoring the field makes the branch compile again so CI can go green. Made-with: Cursor * Replace placeholder release-note content (NONE / N/A) on injection The script previously appended its auto-detected lines INSIDE the ```release-note fence but never displaced template placeholders, so PRs that only had `NONE` ended up with output like: NONE Added `Foo.Bar` configuration setting. Go runtime updated from 1.25.8 to 1.25.9. When the existing fence content is empty or consists only of placeholder tokens (NONE, N/A, NA, dashes — case-insensitive), replace it entirely with the auto-detected entries. User-written human content is still preserved by appending instead. Idempotent: stripping followed by re-injection keeps the placeholder visible when there's nothing to inject, and replaces it again when there is. Made-with: Cursor * Update config-change-checker.yml * Update check_config_changes_ci.py --------- Co-authored-by: Your Name <eva.sarafianou@gmail.com> Co-authored-by: Mattermost Build <build@mattermost.com>	2026-05-19 12:04:25 +03:00