Resolve review findings: core_dumps sysctl, sshd changelog, tag vocabulary, example, php PATH#280
Merged
Merged
Conversation
Stop deploying an own /etc/sysctl.d file plus a sysctl --system handler. The fs.suid_dumpable value is now injected into the kernel_settings role via core_dumps__kernel_settings__sysctl__dependent_var, wired into both core_dumps.yml and setup_basic.yml, so sysctl management stays in a single place. Drop the redundant core_dumps__sysctl_suid_dumpable knob (the role only ever disables core dumps); override via kernel_settings if needed.
…ange The hardened sshd defaults (X11/agent forwarding and TCP keepalives off, lower MaxAuthTries/ClientAliveCountMax, VERBOSE log level) change the behaviour of existing installations on the next run, so the entry belongs under Breaking Changes, not Changed. Add the remediation variables needed to restore the previous behaviour.
…ulary The controlled tag vocabulary was inconsistent in number: certs, containers, networks, modules, plugins and firewalls were plural while user and database were singular. Standardize on plural for count-noun sections (Ansible's common convention): :user -> :users and :database -> :databases across all roles and their READMEs, and align the CONTRIBUTING vocabulary accordingly. Update the breaking-change entry to describe the end state relative to the last release.
The two comments describing example__backup_target and example__conf_tls_protocols documented user-facing variable semantics in the source. That belongs in the README (where both are already covered); comments in the reference role should explain patterns to developers, not duplicate user documentation.
The env[PATH] fix was applied only to the Red Hat pool template, but php-fpm clears the worker environment (clear_env defaults to yes) on every platform. Apply the same env[PATH] to the Debian pool template (shared by Debian and Ubuntu). The value matches Debian's and Ubuntu's ENV_SUPATH, verified on Debian 13 and Ubuntu 24.04.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses review feedback on recently merged commits. One commit per finding.
Findings addressed
core_dumps: route
fs.suid_dumpablethroughkernel_settings(169c23af)The role deployed its own
/etc/sysctl.dfile plus asysctl --systemhandler. sysctl now goes through the dedicated
kernel_settingsrole(injected via
core_dumps__kernel_settings__sysctl__dependent_var, wiredinto
core_dumps.ymlandsetup_basic.yml). The redundantcore_dumps__sysctl_suid_dumpableknob was dropped.sshd hardening is a breaking change (
613b739b)The hardened sshd defaults change existing installations on the next run,
so the changelog entry moved from
ChangedtoBreaking Changes, withremediation variables.
Section tags made plural across the controlled vocabulary (
95b016ff):user/:databasewere singular while:certs/:containers/:networks/:modules/:plugins/:firewallswere plural. Standardized on plural(
:users,:databases) across all roles, READMEs and CONTRIBUTING.example role: no user-facing comments in source (
856fa7ba)Removed variable-semantics comments from
roles/example/defaults/main.yml;the information lives in the README.
php:
env[PATH]fix missing on Debian/Ubuntu (8cddafb3)The php-fpm
env[PATH]fix was Red Hat only. Applied the same fix to theDebian pool template (shared by Debian and Ubuntu); value matches the
distro
ENV_SUPATH, verified on Debian 13 and Ubuntu 24.04.Notes
setup_basicnow pulls inkernel_settings, which requires thefedora.linux_system_rolescollection on the controller.Verification
yamllint clean;
ansible-playbook --syntax-checkon the touched playbooks;php
env[PATH]validated empirically in Debian 13 and Ubuntu 24.04 containers.