-
Notifications
You must be signed in to change notification settings - Fork 23
ci: Add config file for CodeRabbit with custom rules #333
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,232 @@ | ||
| # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json | ||
| # CodeRabbit configuration for linux-system-roles | ||
| # Based on conventions from https://linux-system-roles.github.io/contribute.html | ||
| # This file is managed from https://github.com/linux-system-roles/.github, | ||
| # any manual edits will be overwritten. | ||
|
|
||
| chat: | ||
| art: false | ||
|
|
||
| reviews: | ||
| # Disable fun features | ||
| poem: false | ||
| in_progress_fortune: false | ||
|
|
||
| # Disable auto-features | ||
| auto_apply_labels: false | ||
| auto_assign_reviewers: false | ||
| request_changes_workflow: false | ||
|
|
||
| # Disable additional review features | ||
| sequence_diagrams: false | ||
| estimate_code_review_effort: false | ||
| suggested_labels: false | ||
| high_level_summary_in_walkthrough: false | ||
|
|
||
| # Disable finishing touches | ||
| finishing_touches: | ||
| unit_tests: | ||
| enabled: false | ||
|
|
||
| # Enforce PR title and description requirements | ||
| pre_merge_checks: | ||
| title: | ||
| mode: "warning" | ||
| requirements: | | ||
| PR title MUST follow Conventional Commits format: | ||
| - Format: <type>: <description> or <type>!: <description> for breaking changes | ||
| - Valid types: Refer to the 'type-enum' rule in .commitlintrc.js file for the complete list of allowed types | ||
| - Examples: | ||
| - "feat: Add backup functionality" | ||
| - "fix: Correct OSTree package installation" | ||
| - "fix!: Remove deprecated variable (breaking change)" | ||
|
|
||
| custom_checks: | ||
| - mode: "warning" | ||
| name: "Description Format" | ||
| instructions: | | ||
| PR description MUST follow the template structure from .github/pull_request_template.md: | ||
| - Must contain "Enhancement:" or "Feature:" section describing what changed | ||
| - Must contain "Reason:" section explaining why the change was needed | ||
| - Must contain "Result:" section describing the outcome or impact | ||
| - Can contain optional "Issue Tracker Tickets (Jira or BZ if any):" section | ||
|
|
||
| Example: | ||
| ``` | ||
| Feature: Introduce the certificate_secure_logging variable | ||
|
|
||
| Reason: Currently, all sensitive tasks use hard-coded no_log: true, which makes debugging difficult. | ||
|
|
||
| Result: Users can now set certificate_secure_logging: false for debugging while maintaining secure defaults. | ||
|
|
||
| Issue Tracker Tickets (Jira or BZ if any): RHEL-12345 | ||
| ``` | ||
|
|
||
| path_instructions: | ||
| # ======================================== | ||
| # Ansible Tasks - Core role logic | ||
| # ======================================== | ||
| - path: "tasks/**/*.yml" | ||
| instructions: | | ||
| **no_log patterns:** | ||
| - For sensitive data (credentials, secrets, passwords, keys): | ||
| ```yaml | ||
| - name: Task with sensitive data | ||
| ansible.builtin.command: sensitive command | ||
| no_log: "{{ certificate_secure_logging }}" | ||
| ``` | ||
| Ensure `certificate_secure_logging: true` is defined in defaults/main.yml | ||
|
|
||
| - For facts modules (package_facts, service_facts): | ||
| ```yaml | ||
| - name: Gather package facts | ||
| ansible.builtin.package_facts: | ||
| no_log: "{{ ansible_verbosity < 3 }}" | ||
| ``` | ||
| This hides verbose facts output unless running with -vvv or higher verbosity | ||
|
|
||
| - NEVER use hardcoded `no_log: true` - always parametrize | ||
|
|
||
| **Package installation (OSTree compatibility):** | ||
| - ALWAYS use `ansible.builtin.package` module (NEVER yum, dnf, or apt) | ||
| - For Red Hat family systems: MUST include `use:` parameter for OSTree compatibility | ||
| - For other systems (apt, etc.): OSTree `use:` parameter NOT needed | ||
|
|
||
| **Red Hat family pattern (rpm/dnf/yum based):** | ||
| ```yaml | ||
| - name: Install packages | ||
| ansible.builtin.package: | ||
| name: "{{ __certificate_packages }}" | ||
| state: present | ||
| use: "{{ (__certificate_is_ostree | d(false)) | ternary('ansible.posix.rhel_rpm_ostree', omit) }}" | ||
| ``` | ||
|
|
||
| **Non-Red Hat systems (apt based) - no OSTree handling needed:** | ||
| ```yaml | ||
| - name: Install packages | ||
| ansible.builtin.package: | ||
| name: "{{ __certificate_packages }}" | ||
| state: present | ||
| when: ansible_facts["pkg_mgr"] == 'apt' | ||
| ``` | ||
|
|
||
| **Pattern explanation:** | ||
| - `__certificate_is_ostree` detects OSTree/rpm-ostree systems (RHEL/CentOS/Fedora variants) | ||
| - This variable should already exist in vars/main.yml - do NOT suggest adding it | ||
| - `| d(false)` provides safe default if variable undefined | ||
| - `ternary()` selects `ansible.posix.rhel_rpm_ostree` for OSTree, `omit` otherwise | ||
| - `omit` removes the parameter entirely on non-OSTree Red Hat systems | ||
|
|
||
| **Third-party collections:** | ||
| - Avoid using third-party collections (community.general, community.crypto, etc.) | ||
| - Use ansible.builtin modules or command module instead | ||
|
|
||
| **Referencing other system roles:** | ||
| - Use FQCN: `fedora.linux_system_roles.certificate` | ||
|
|
||
| **Idempotency:** | ||
| - Tasks must be idempotent - safe to run multiple times without unintended changes | ||
| - Use proper state parameters (present/absent, started/stopped, etc.) | ||
| - Command/shell tasks should use creates/removes or changed_when to avoid false changes | ||
| - Example: | ||
| ```yaml | ||
| - name: Initialize database | ||
| ansible.builtin.command: /usr/bin/initialize-db | ||
| args: | ||
| creates: /var/lib/db/initialized.flag | ||
| ``` | ||
|
|
||
| **Check mode support:** | ||
| - Critical tasks should support check mode (--check flag) | ||
| - Use check_mode: false only when absolutely necessary (e.g., fact gathering) | ||
| - Test that role works with --check --diff | ||
|
|
||
| **Test coverage requirement:** | ||
| - When adding new tasks to tasks/main.yml or creating new task files, verify that corresponding test coverage exists in tests/ | ||
| - New functionality MUST include test files (tests/tests_*.yml) that exercise the new code paths | ||
| - Tests should verify both success scenarios and failure/edge cases | ||
| - If this PR adds new tasks but does not include new or updated tests, flag it and request test coverage | ||
|
|
||
| # ======================================== | ||
| # Handlers | ||
| # ======================================== | ||
| - path: "handlers/**/*.yml" | ||
| instructions: | | ||
| - Handlers with sensitive data must use `no_log: "{{ certificate_secure_logging }}"` | ||
|
|
||
| # ======================================== | ||
| # Test Playbooks | ||
| # ======================================== | ||
| - path: "tests/tests_*.yml" | ||
| instructions: | | ||
| **CRITICAL: Role invocation pattern:** | ||
| - NEVER use `ansible.builtin.include_role` directly | ||
| - NEVER use `ansible.builtin.import_role` directly | ||
| - NEVER use `roles:` keyword | ||
| - ALWAYS use the centrally managed wrapper: | ||
| ```yaml | ||
| - name: Run role | ||
| ansible.builtin.include_tasks: tasks/run_role_with_clear_facts.yml | ||
| vars: | ||
| certificate_<parameter>: <value> | ||
| ``` | ||
|
|
||
| **Test quality requirements:** | ||
| - Tests should verify both success and failure scenarios | ||
| - Use assert module to verify expected state after role execution | ||
| - Include cleanup tasks to ensure tests are rerunnable | ||
| - Tests should be idempotent - running twice should not cause failures | ||
| - Example verification: | ||
| ```yaml | ||
| - name: Verify service is running | ||
| ansible.builtin.assert: | ||
| that: | ||
| - ansible_facts.services['mssql-server.service'].state == 'running' | ||
| fail_msg: "SQL Server service is not running" | ||
| ``` | ||
|
|
||
| # ======================================== | ||
| # Templates | ||
| # ======================================== | ||
| - path: "templates/**/*.j2" | ||
| instructions: | | ||
| **Required headers (in this order):** | ||
| 1. ansible_managed header: `{{ ansible_managed | comment }}` | ||
| 2. Role fingerprint: `{{ "system_role:certificate" | comment(prefix="", postfix="") }}` | ||
|
|
||
| # ======================================== | ||
| # Variable Definitions - defaults/ | ||
| # ======================================== | ||
| - path: "defaults/**/*.yml" | ||
| instructions: | | ||
| - All variables MUST be prefixed with `certificate_` | ||
| - All variables MUST be stored in the file defaults/main.yml, Ansible | ||
| doesn't include variables from other files. | ||
| - These are user-facing API variables | ||
| - For every new variable introduced in this file, verify that it is | ||
| documented in README.md with a description and a usage example. | ||
| If it is missing from README.md, flag it and request that it be added. | ||
|
|
||
| # ======================================== | ||
| # Variable Definitions - vars/ | ||
| # ======================================== | ||
| - path: "vars/**/*.yml" | ||
| instructions: | | ||
| - Internal variables MUST be prefixed with `__certificate_` | ||
| - User-facing variables belong in defaults/main.yml, not here | ||
|
|
||
| # ======================================== | ||
| # Python Code | ||
| # ======================================== | ||
| - path: "**/*.py" | ||
| instructions: | | ||
| - Must follow PEP 8 and be formatted with Python Black | ||
| - Run `tox -e black,flake8` before committing | ||
|
|
||
| # ======================================== | ||
| # Documentation | ||
| # ======================================== | ||
| - path: "README.md" | ||
| instructions: | | ||
| - Document all new user-facing variables | ||
| - Include usage examples for new functionality | ||
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Clarify scope of the statement about Ansible not including variables from other files.
The phrase
Ansible doesn't include variables from other files.is accurate for role defaults (onlydefaults/main.ymlis auto-loaded) but can be misread as a general rule about all Ansible variables. Please narrow it to something likeAnsible only auto-loads role defaults from defaults/main.ymlto avoid confusion with mechanisms likeinclude_vars.