add the commit_changes role #686
Conversation
|
from change #686:
|
1 similar comment
|
from change #686:
|
📝 WalkthroughWalkthroughA new Ansible role, Changes
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
✨ Finishing Touches
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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.
There was a problem hiding this comment.
ok, good idea.
There was a problem hiding this comment.
@fredericlepied, great to hear you're on board with using the git_config module! It'll definitely make the role more robust and easier to maintain. 🐰
(ﾉ◕ヮ◕)ﾉ*:･ﾟ✧ ♪～(´ε｀ )
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| - 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. |
|
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. |
|
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:
|
|
recheck |
|
from change #686:
|
1 similar comment
|
from change #686:
|
|
Build failed. ❌ dci-rpm-build-el8 FAILURE in 2m 20s |
|
from change #686:
|
1 similar comment
|
from change #686:
|
|
Build failed. ❌ dci-rpm-build-el8 FAILURE in 2m 24s |
|
from change #686:
|
1 similar comment
|
from change #686:
|
|
Build succeeded. ✔️ dci-rpm-build-el8 SUCCESS in 2m 51s |
|
from change #686:
|
1 similar comment
|
from change #686:
|
|
Build succeeded. ✔️ dci-rpm-build-el8 SUCCESS in 2m 50s |
|
from change #686:
|
1 similar comment
|
from change #686:
|
|
Build succeeded. ✔️ dci-rpm-build-el8 SUCCESS in 2m 53s |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
roles/commit_changes/README.md (2)
1-4: Add a Requirements section.The README jumps directly into the role description without listing prerequisites. Introduce a Requirements heading to note that Git CLI must be installed and available in
PATH.
7-13: Expand variable docs with types, defaults, and an example.The table omits data types and explicit defaults (
null/None) for required parameters. Consider adding a Type column and default values, plus an Example snippet showing how to call the role with all required vars.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
README.md(1 hunks)ansible-collection-redhatci-ocp.spec(2 hunks)galaxy.yml(1 hunks)roles/commit_changes/README.md(1 hunks)roles/commit_changes/tasks/main.yml(1 hunks)tests/integration/targets/commit_changes/commit_changes.yml(1 hunks)tests/integration/targets/commit_changes/runme.sh(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- ansible-collection-redhatci-ocp.spec
- galaxy.yml
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/integration/targets/commit_changes/runme.sh
- README.md
- roles/commit_changes/tasks/main.yml
| - name: Run the role under test | ||
| import_role: | ||
| name: redhatci.ocp.commit_changes | ||
|
|
||
| - name: Verify that commit was made | ||
| ansible.builtin.assert: | ||
| that: | ||
| - _cc_result.changed | ||
|
|
There was a problem hiding this comment.
Capture the role’s result before asserting.
You’re importing the role without registering its output, so _cc_result will be undefined. Change to:
- name: Run the role under test
include_role:
name: redhatci.ocp.commit_changes
register: _cc_resultThis ensures you can assert on _cc_result.changed.
🤖 Prompt for AI Agents
In tests/integration/targets/commit_changes/commit_changes.yml around lines 54
to 62, the role is imported without capturing its result, so the variable
_cc_result is undefined when asserting. Replace the import_role task with
include_role and register its output to _cc_result. This allows the subsequent
assertion to correctly check if the role made changes by referencing
_cc_result.changed.
| - name: Assert git log message matches cc_message | ||
| ansible.builtin.assert: | ||
| that: | ||
| - git_log.stdout.strip() == cc_message | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a Jinja2 filter instead of .strip().
Ansible’s Jinja2 may not allow direct Python method calls. Replace:
- git_log.stdout.strip() == cc_messagewith:
- git_log.stdout | trim == cc_message🤖 Prompt for AI Agents
In tests/integration/targets/commit_changes/commit_changes.yml around lines 69
to 73, replace the Python string method call git_log.stdout.strip() with the
Jinja2 filter git_log.stdout | trim to ensure compatibility with Ansible's
templating engine. Update the assertion to use the Jinja2 trim filter for
stripping whitespace instead of the .strip() method.
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 7 days |
|
from change #686:
|
1 similar comment
|
from change #686:
|
|
Build succeeded. ✔️ dci-rpm-build-el8 SUCCESS in 2m 48s |
SUMMARY
ISSUE TYPE
Tests
Test-Hint: no-check