add the commit_changes role #686
Conversation
|
from change #686:
|
1 similar comment
|
from change #686:
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis change introduces a new Ansible role, Changes
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
roles/commit_changes/tasks/main.yml (1)
30-37: 🛠️ Refactor suggestionAvoid shell‐splitting issues by using list/argv form.
Passing the commit message as a single argument prevents injection and ensures messages with spaces work:- name: Commit changes ansible.builtin.command: argv: - git - commit - -m - "{{ commit_message }}" args: chdir: "{{ commit_directory }}" register: commit_result failed_when: commit_result.rc > 1 changed_when: commit_result.rc == 0
🧹 Nitpick comments (7)
tests/integration/inventory (1)
2-2: Avoid hardcoding local paths in inventory
Theansible_python_interpreteris pointed at a user-specific venv path. For portability, consider using a generic interpreter (e.g./usr/bin/env python3) or parameterizing via an environment variable or inventory var.roles/commit_changes/defaults/main.yml (1)
1-4: Add newline at end of file
YAML lint flagged a missing newline at EOF. Please ensure there’s a trailing newline to comply with common YAML style guidelines.🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 4-4: no new line character at the end of file
(new-line-at-end-of-file)
tests/integration/integration_config.yml (1)
1-7: Ensure trailing newline for YAML file
Add a newline at the end of this file to satisfy YAML style checks and avoid “no new line at end of file” warnings.tests/integration/targets/commit_changes/commit_changes.yml (1)
1-7: Optimize playbook for speed
Since this integration test doesn’t need facts, you could addgather_facts: nounder the play to reduce runtime.roles/commit_changes/README.md (1)
1-3: Unify Markdown heading style.
Currently the top-level header uses Setext while the others use ATX, which triggers MD003. For consistency, convert to ATX:- commit_changes - =============== + # commit_changesroles/commit_changes/tasks/main.yml (1)
1-7: Add a check that the directory is a Git repo.
Right now we only assert the variables exist. It’s safer to verify thatcommit_directory/.gitactually exists before proceeding:- name: Check for Git repository ansible.builtin.stat: path: "{{ commit_directory }}/.git" register: git_dir - name: Fail if not a Git repository ansible.builtin.assert: that: - git_dir.exists fail_msg: "Directory {{ commit_directory }} is not a Git repository"tests/integration/targets/commit_changes/tasks/main.yml (1)
3-7: Ensure directory removal is recursive if needed.
To guarantee cleanup of non-empty dirs, addrecurse: yes:- name: Cleanup target directory ansible.builtin.file: path: "{{ commit_directory }}" state: absent recurse: yes
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
README.md(1 hunks)roles/commit_changes/README.md(1 hunks)roles/commit_changes/defaults/main.yml(1 hunks)roles/commit_changes/tasks/main.yml(1 hunks)tests/integration/integration_config.yml(1 hunks)tests/integration/inventory(1 hunks)tests/integration/targets/commit_changes/commit_changes.yml(1 hunks)tests/integration/targets/commit_changes/runme.sh(1 hunks)tests/integration/targets/commit_changes/tasks/main.yml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
roles/commit_changes/defaults/main.yml
[error] 4-4: no new line character at the end of file
(new-line-at-end-of-file)
🪛 markdownlint-cli2 (0.17.2)
roles/commit_changes/README.md
6-6: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
15-15: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
19-19: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
🔇 Additional comments (5)
README.md (1)
50-50: Role entry added correctly
Thecommit_changesrole is listed in the Roles table with an accurate link and description. Positioning and formatting match existing entries.roles/commit_changes/README.md (3)
6-14: Variables section is clear and accurate.
The table correctly documents required and optional inputs.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
6-6: Heading style
Expected: setext; Actual: atx(MD003, heading-style)
15-18: Dependencies section is correct.
No external requirements—documentation matches implementation.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
15-15: Heading style
Expected: setext; Actual: atx(MD003, heading-style)
19-21: License section is appropriate.
GNU GPLv3 is properly declared.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
19-19: Heading style
Expected: setext; Actual: atx(MD003, heading-style)
roles/commit_changes/tasks/main.yml (1)
25-29: Staging changes looks good.
Usinggit add --all .withchdircorrectly stages all file states.
| dir=$(dirname "$0") | ||
|
|
||
| exec ansible-playbook -i $dir/../../inventory -e @$dir/../../integration_config.yml commit_changes.yml | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure playbook path and variables are quoted and correctly referenced.
The script determines its own directory but then runs commit_changes.yml relative to the current working directory, which may cause “file not found” errors if you invoke the script from elsewhere. Also, the inventory and vars paths should be quoted to handle spaces.
Apply this diff:
- dir=$(dirname "$0")
-
- exec ansible-playbook -i $dir/../../inventory -e @$dir/../../integration_config.yml commit_changes.yml
+ dir="$(dirname "$0")"
+
+ # run from the role’s test directory
+ cd "$dir"
+
+ exec ansible-playbook \
+ "$dir/commit_changes.yml" \
+ -i "$dir/../../inventory" \
+ -e "@$dir/../../integration_config.yml"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| dir=$(dirname "$0") | |
| exec ansible-playbook -i $dir/../../inventory -e @$dir/../../integration_config.yml commit_changes.yml | |
| dir="$(dirname "$0")" | |
| # run from the role’s test directory | |
| cd "$dir" | |
| exec ansible-playbook \ | |
| "$dir/commit_changes.yml" \ | |
| -i "$dir/../../inventory" \ | |
| -e "@$dir/../../integration_config.yml" |
🤖 Prompt for AI Agents
In tests/integration/targets/commit_changes/runme.sh around lines 3 to 6, the
ansible-playbook command uses unquoted paths and references the playbook
relative to the current directory instead of the script directory. To fix this,
quote all path variables to handle spaces and reference commit_changes.yml using
the script directory variable to ensure the correct file is found regardless of
the current working directory.
roles/commit_changes/tasks/main.yml
Outdated
| - name: Set git author name | ||
| ansible.builtin.command: git config user.name "{{ commit_author_name }}" | ||
| args: | ||
| chdir: "{{ commit_directory }}" | ||
| when: | ||
| - commit_author_name is defined | ||
| - commit_author_name | length > 0 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use the idempotent git_config module for author settings.
Replacing raw git config commands with ansible.builtin.git_config improves idempotency and readability:
- name: Set git author name
ansible.builtin.git_config:
name: user.name
value: "{{ commit_author_name }}"
scope: local
repo: "{{ commit_directory }}"
when:
- commit_author_name is defined
- commit_author_name | length > 0
- name: Set git author email
ansible.builtin.git_config:
name: user.email
value: "{{ commit_author_email }}"
scope: local
repo: "{{ commit_directory }}"
when:
- commit_author_email is defined
- commit_author_email | length > 0Also applies to: 17-23
🤖 Prompt for AI Agents
In roles/commit_changes/tasks/main.yml lines 9 to 15 and also lines 17 to 23,
replace the ansible.builtin.command tasks that run raw git config commands with
the ansible.builtin.git_config module. Use the git_config module's parameters:
name for the config key (user.name or user.email), value for the corresponding
variable, scope set to local, and repo set to commit_directory. Keep the
existing when conditions to check if the variables are defined and non-empty.
This change ensures idempotency and improves readability.
| - name: Verify that commit was made | ||
| ansible.builtin.assert: | ||
| that: | ||
| - commit_result.changed | ||
|
|
||
| - name: Verify commit message is correct | ||
| ansible.builtin.command: git log -1 --pretty=%B | ||
| args: | ||
| chdir: "{{ commit_directory }}" | ||
| register: git_log | ||
|
|
||
| - name: Assert git log message matches commit_message | ||
| ansible.builtin.assert: | ||
| that: | ||
| - git_log.stdout.strip() == commit_message | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expand tests to cover author overrides.
We verify that a commit is made and the message matches, but we don’t assert commit_author_name/email behavior. Consider adding:
- name: Get latest commit author name
ansible.builtin.command:
argv:
- git
- log
- -1
- --pretty=%an
args:
chdir: "{{ commit_directory }}"
register: git_author
- name: Assert commit author name
ansible.builtin.assert:
that:
- git_author.stdout.strip() == commit_author_name
when: commit_author_name is definedPlace these after the message assertion to fully cover optional author overrides.
🤖 Prompt for AI Agents
In tests/integration/targets/commit_changes/tasks/main.yml around lines 52 to
67, the current tests verify commit creation and message correctness but do not
check commit author name or email overrides. Add tasks after the commit message
assertion to run a git command retrieving the latest commit author name,
register its output, and assert that it matches the expected commit_author_name
variable. Include a conditional to run this assertion only if commit_author_name
is defined, ensuring coverage of optional author overrides.
|
from change #686:
|
1 similar comment
|
from change #686:
|
|
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
|
from change #686:
|
1 similar comment
|
from change #686:
|
|
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
SUMMARY
ISSUE TYPE
Tests
Test-Hint: no-check
Depends-on: #676