Conversation
WalkthroughRenamed many registered variables and set_facts to role-prefixed names, refactored several loop/item structures and systemd/unit lists, introduced default flag Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Potential focus areas:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
ansible/roles/ovos_hardware_mark1/tasks/config.yml(1 hunks)ansible/roles/ovos_hardware_mark2/tasks/firmware.yml(1 hunks)ansible/roles/ovos_hardware_mark2/tasks/prepare.yml(1 hunks)ansible/roles/ovos_installer/tasks/finalize.yml(2 hunks)ansible/roles/ovos_installer/tasks/ovos.yml(1 hunks)ansible/roles/ovos_installer/tasks/sound.yml(2 hunks)ansible/roles/ovos_installer/tasks/timezone.yml(1 hunks)ansible/roles/ovos_installer/tasks/video.yml(2 hunks)ansible/roles/ovos_installer/tasks/virtualenv/bus.yml(1 hunks)ansible/roles/ovos_installer/tasks/virtualenv/systemd.yml(1 hunks)
⏰ 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). (2)
- GitHub Check: ansible-lint
- GitHub Check: ansible-lint
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ansible/roles/ovos_installer/tasks/tuning/numa.yml (1)
11-20: Update variable references in three files to complete the refactoring.Verification confirms undefined variable references that will cause runtime failures. The following files still reference the old
_boot_directoryvariable which is no longer being set:
ansible/roles/ovos_hardware_mark2/tasks/touchscreen.yml(lines 4, 10)ansible/roles/ovos_hardware_mark1/tasks/config.yml(line 13)Update these to use the role-specific variable names:
ovos_hardware_mark2_boot_directoryandovos_hardware_mark1_boot_directoryrespectively, consistent with the refactoring already completed in other files.ansible/roles/ovos_hardware_mark1/tasks/config.yml (1)
13-13: Variable reference inconsistency: line 13 still uses old variable name.Line 13 references
{{ _boot_directory }}, but the variable was renamed toovos_hardware_mark1_boot_directoryat line 9. This will cause a template rendering error. Lines 23 and 32 were correctly updated to use the new variable name.Apply this diff to fix the inconsistency:
- name: Manage TTY and soundcard overlays ansible.builtin.lineinfile: - path: "{{ _boot_directory }}/config.txt" + path: "{{ ovos_hardware_mark1_boot_directory }}/config.txt" regexp: "^{{ item }}" line: "{{ item }}"
🧹 Nitpick comments (3)
setup.sh (1)
66-66: Redundant assignment:ansible_cleaningalready initialized at line 26.Line 66 reassigns
ansible_cleaning="false", but the variable was already initialized with the same value at line 26. If the intention is to reset the value unconditionally in this branch, the comment is acceptable; otherwise, this assignment can be removed.ansible/roles/ovos_installer/tasks/virtualenv/systemd.yml (1)
92-102: Confirm ovos_installer_configuration availability for systemd restart logic
ovos_installer_systemd_statederives fromovos_installer_configuration.changed, which assumes thatovos_installer_configurationis always defined before this task runs. If there is any code path where that register might be skipped, this could raise an undefined-variable error at runtime. Otherwise, the enable/start loop (including scopes, users, and per-profile state) looks sound.If you expect some runs to skip the configuration task, consider guarding this task with a
when: ovos_installer_configuration is definedor providing a default (| default(false)) in the expression to make it more robust.Also applies to: 104-152
ansible/roles/ovos_installer/tasks/virtualenv/venv.yml (1)
32-38: Clarify CPU capability condition with parenthesesThe two venv-creation tasks cleanly separate the “with tflite_runtime” and “without tflite_runtime” paths based on
ovos_installer_cpu_is_capable. To avoid any ambiguity in Jinja filter precedence and make the intent explicit, the secondwhenwould be clearer with parentheses:- when: not ovos_installer_cpu_is_capable | bool + when: not (ovos_installer_cpu_is_capable | bool)Functionally this should be equivalent, but the explicit grouping is easier to read and less error-prone.
Also applies to: 40-45
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
.github/workflows/scenarios-ubuntu2404.yml(2 hunks).github/workflows/sync_tx.yml(0 hunks)ansible/roles/ovos_hardware_mark1/handlers/main.yml(1 hunks)ansible/roles/ovos_hardware_mark1/tasks/config.yml(3 hunks)ansible/roles/ovos_hardware_mark1/tasks/prepare.yml(2 hunks)ansible/roles/ovos_hardware_mark2/handlers/main.yml(1 hunks)ansible/roles/ovos_hardware_mark2/meta/main.yml(1 hunks)ansible/roles/ovos_hardware_mark2/tasks/firmware.yml(1 hunks)ansible/roles/ovos_hardware_mark2/tasks/prepare.yml(1 hunks)ansible/roles/ovos_hardware_mark2/tasks/vocalfusion.yml(4 hunks)ansible/roles/ovos_installer/defaults/main.yml(1 hunks)ansible/roles/ovos_installer/tasks/docker/common.yml(2 hunks)ansible/roles/ovos_installer/tasks/docker/composer.yml(5 hunks)ansible/roles/ovos_installer/tasks/docker/setup-Debian.yml(3 hunks)ansible/roles/ovos_installer/tasks/docker/setup-RedHat.yml(1 hunks)ansible/roles/ovos_installer/tasks/ovos.yml(4 hunks)ansible/roles/ovos_installer/tasks/sound.yml(5 hunks)ansible/roles/ovos_installer/tasks/timezone.yml(1 hunks)ansible/roles/ovos_installer/tasks/tuning/governor.yml(2 hunks)ansible/roles/ovos_installer/tasks/tuning/io.yml(1 hunks)ansible/roles/ovos_installer/tasks/tuning/numa.yml(1 hunks)ansible/roles/ovos_installer/tasks/video.yml(3 hunks)ansible/roles/ovos_installer/tasks/virtualenv/gui.yml(1 hunks)ansible/roles/ovos_installer/tasks/virtualenv/systemd.yml(2 hunks)ansible/roles/ovos_installer/tasks/virtualenv/venv.yml(4 hunks)ansible/roles/ovos_installer/telemetry.yml(2 hunks)ansible/roles/ovos_installer/templates/virtualenv/core-requirements.txt.j2(1 hunks)ansible/roles/ovos_installer/templates/virtualenv/skills-requirements.txt.j2(1 hunks)setup.sh(1 hunks)tests/molecule/ovos_installer/roles/test_ovos_installer/defaults/main.yml(1 hunks)tests/molecule/ovos_installer/roles/test_ovos_installer/tasks/configuration.yml(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/sync_tx.yml
✅ Files skipped from review due to trivial changes (3)
- ansible/roles/ovos_installer/tasks/tuning/io.yml
- .github/workflows/scenarios-ubuntu2404.yml
- ansible/roles/ovos_installer/tasks/virtualenv/gui.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- ansible/roles/ovos_installer/tasks/timezone.yml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-09-25T01:36:33.089Z
Learnt from: Erotemic
Repo: OpenVoiceOS/ovos-installer PR: 151
File: utils/common.sh:295-302
Timestamp: 2024-09-25T01:36:33.089Z
Learning: Defaulting `USE_UV` to `true` is acceptable as it significantly speeds up installations in the OVOS installer.
Applied to files:
ansible/roles/ovos_installer/defaults/main.yml
🔇 Additional comments (42)
ansible/roles/ovos_installer/telemetry.yml (2)
13-14: ✓ Variable renaming and until condition properly updated.The registered variable is consistently renamed to
ovos_installer_isp_dataand theuntilcondition correctly references the new variable name.
30-31: ✓ Variable renaming and until condition properly updated.The registered variable is consistently renamed to
ovos_installer_push_telemetry_dataand theuntilcondition correctly references the new variable name.setup.sh (1)
25-28: Good defensive initialization to prevent unbound-variable errors.Initializing
ansible_cleaning,ansible_tags, andansible_debugearly ensures these variables are always defined, preventing errors ifset -uis later enabled or if conditional branches are skipped. The placement before the DEBUG check is correct, and the array syntax is proper for Bash.ansible/roles/ovos_installer/tasks/video.yml (3)
2-9: Approve task name update and user group loop structure.The task name simplification (removing templated
{{ ovos_installer_user }}) aligns with ansible-lint best practices. However, theusermodule loop structure warrants verification:groups: audiois static while the loop iterates overvideoandrender, which will add the user to the audio group once per iteration (redundantly).Please verify via
git diffwhether this loop/groups pattern existed in the previous version or if it's a new structure introduced by this PR. If new, consider refactoring to:- name: Add installer user to video and render groups ansible.builtin.user: name: "{{ ovos_installer_user }}" groups: "{{ item }}" append: true loop: - audio - video - render
31-46: Consistent variable renaming for wayland socket detection.Renamed
_ovos_wayland_display→ovos_installer_ovos_wayland_displaywith all downstream references (lines 43, 45) properly updated. This naming convention aligns with ansible-lint recommendations for registered variable scoping and naming clarity.
48-80: Consistent variable renaming for DISPLAY server detection.Renamed
_ovos_display_server_number→ovos_installer_ovos_display_server_numberwith all downstream references (lines 79–80) properly updated. The when condition correctly checksstdout | length > 0to validate non-empty output.ansible/roles/ovos_installer/tasks/sound.yml (3)
101-102: Reference to renamed variable correctly updated.The
.asoundrcgeneration task now correctly referencesovos_installer_detect_sound_serveron lines 101–102, maintaining the original logic for bothpcm.!defaultandctl.!defaultassignments.Also applies to: 98-108
32-45: Task names simplified; logic preserved.Task names have been updated from including the user variable (e.g., "Add {{ ovos_installer_user }} to audio group") to more generic phrasing ("Add installer user to audio group"). The when conditions remain logically intact, including the reformatted condition on lines 56–57 for the pipewire group task.
Also applies to: 47-62
110-116: Removal task now properly gated for uninstall scenarios.The .asoundrc removal task now includes the
when: ovos_installer_cleaning | boolcondition and theuninstalltag, ensuring the file is only removed during cleanup operations.ansible/roles/ovos_installer/tasks/tuning/numa.yml (2)
2-5: Variable naming refactor: boot config status register.The register variable has been renamed from
_boot_config_statustoovos_installer_boot_config_status, adopting an explicit namespace prefix instead of an underscore-only private scope convention. This aligns with Ansible best practices and the PR's linting fixes.
7-9: Variable naming refactor: boot directory fact.The set_fact task correctly updates both the task name and variable assignment to use the new namespaced prefix
ovos_installer_boot_directory. The conditional reference toovos_installer_boot_config_status.stat.existscorrectly matches the registered variable from line 5.tests/molecule/ovos_installer/roles/test_ovos_installer/defaults/main.yml (1)
2-7: LGTM — Variable renaming is clean and consistent.The renaming from
ovos_installer_*totest_ovos_installer_*properly scopes these variables to the test role, avoiding potential conflicts with production installer variables.tests/molecule/ovos_installer/roles/test_ovos_installer/tasks/configuration.yml (4)
10-17: Consistent variable usage and assertions for container configuration.The assertions for the container config directory properly validate existence, type, ownership, and permissions using the updated variable names.
24-31: Consistent variable usage and assertions for container mycroft.conf.The assertions for the configuration file properly validate existence (negating
isdir), ownership, and stricter file permissions (0600) using the updated variable names.
41-48: Consistent variable usage and assertions for virtualenv configuration.The assertions for the virtualenv config directory mirror the container block logic correctly with updated variable names.
55-62: Consistent variable usage and assertions for virtualenv mycroft.conf.The assertions for the virtualenv configuration file properly validate all required properties using updated variable names.
ansible/roles/ovos_installer/tasks/tuning/governor.yml (2)
20-20: Module replacement looks appropriate for linting fix.Switching from
community.general.zyppertoansible.builtin.packageis a valid linting improvement and reduces dependency on the community.general collection for this task. Verify thatansible.builtin.packagehandles SUSE package management correctly in your testing environment.
37-45: Variable renaming and references verified as correct.Verification confirms that
_ovos_installer_cpu_governor_configis properly defined inansible/roles/ovos_installer/vars/main.yml(line 7) with the value/etc/default/cpu_governor. All references in the task file at lines 37 and 42 are correctly maintained. The variable renaming toovos_installer_cpu_governor_configfollows Ansible conventions and is consistently applied throughout.ansible/roles/ovos_installer/templates/virtualenv/skills-requirements.txt.j2 (1)
7-9: GGwave skill inclusion correctly gated by new feature flag
ovos-skill-ggwaveis now conditioned onovos_installer_enable_ggwaveandovos_installer_profile != "server", which matches how the systemd unit is gated and centralizes the enable/disable logic in the flag. No issues spotted here.ansible/roles/ovos_installer/defaults/main.yml (1)
12-12: ovos_installer_enable_ggwave default is well-scopedThe default for
ovos_installer_enable_ggwaveties enablement toovos_installer_channelin['stable', 'testing']and architectures in['x86_64', 'aarch64']. This centralizes the previous architecture/channel gating cleanly. Looks good.ansible/roles/ovos_installer/templates/virtualenv/core-requirements.txt.j2 (1)
24-26: Core ggwave plugin is correctly tied to ovos_installer_enable_ggwaveSwitching the condition to
ovos_installer_enable_ggwavekeeps the logic in one place and matches the new flag semantics. No functional issues identified.ansible/roles/ovos_installer/tasks/docker/setup-RedHat.yml (1)
4-12: RedHat Docker repo detection and permissions look correctUsing
ovos_installer_os_distributionto choose betweencentosandfedorain the Docker repo URL is straightforward and keeps the logic in one place. Addingmode: "0644"fordocker-ce.repois appropriate for a repo file. No issues spotted.ansible/roles/ovos_installer/tasks/docker/setup-Debian.yml (1)
4-5: Debian/Ubuntu Docker repo selection and repo stanza are consistent
ovos_installer_os_distributioncleanly distinguishes Debian ('debian') from everything else ('ubuntu'), and both the GPG key URL andapt_repositorynow use that variable. The multi-linerepostring still produces a validdeb ...line. No problems found.Also applies to: 15-19, 33-36
ansible/roles/ovos_installer/tasks/virtualenv/systemd.yml (2)
13-24: Systemd unit copy paths and notify scope are coherentUsing
ovos_installer_systemd_path_user/ovos_installer_systemd_path_systemto route units to the correct directories and derivingovos_installer_notify_scopefromitem.user(Reload SystemdvsReload Systemd User) is a clear improvement. The per-unitstateexpressions match the intended profiles/features, andwhen: item.state | boolis the right pattern for filtering the loop. No functional issues observed.Also applies to: 25-87
168-203: Uninstall stop/disable loop change maintains behaviorThe expanded stop/disable loop lists all relevant OVOS and HiveMind units explicitly with appropriate
scopeanduservalues. Combined withfailed_when: ovos_installer_systemd_stop is not defined, this keeps the uninstall path tolerant of missing units. Behavior is consistent with the rest of the systemd handling.ansible/roles/ovos_installer/tasks/virtualenv/venv.yml (5)
9-30: Requirements templating loop and per-profile gating look correctThe new
loopstructure withfile/dest/stateandwhen: item.state | boolcleanly drives which requirements files are rendered for each profile/feature (core, GUI, skills, extra-skills, listener, satellite, server). The destinations under/tmpalign with the later install step and final cleanup. No issues here.
47-53: GGwave library installation is correctly tied to the enable flagThe
moreati.uv.piptask forggwaveis guarded bywhen: ovos_installer_enable_ggwave | bool, which lines up with how the flag is defined in defaults and used in the requirements/templates. This looks consistent.
63-65: Install-venv retry loop and item gating are soundRenaming the register variable to
ovos_installer_install_venvand usinguntil: ovos_installer_install_venv is successis clear. The loop over/tmp/*-requirements.txtwith per-itemstateandwhen: item.state | boolcorrectly reuses the earlier profile/feature gating. The retry settings (5 retries, 5s delay) are reasonable.Also applies to: 68-82
90-113: Venv ownership and .bashrc updates remain idempotentChanging the task name for the chown operation and the
.bashrcupdates improves readability; the underlying use ofchanged_when: falseandlineinfilewithstate: "{{ ovos_installer_uninstall }}"and i2c-based gating is consistent with the existing behavior. No issues identified.
160-176: Venv and requirements cleanup covers new /tmp filesThe uninstall cleanup task now removes the ovos venv, all
/tmp/*-requirements.txtfiles used earlier, device-specific paths under/opt, and theovos-messagebuswrapper in the user’s local bin. This matches the new layout introduced above and keeps the system tidy on uninstall.ansible/roles/ovos_installer/tasks/docker/common.yml (2)
2-7: Docker group membership task rename is fineRenaming the task to “Add installer user to docker group” while keeping the
ansible.builtin.userparameters unchanged improves clarity without affecting behavior. All good here.
27-35: Repository cloning conditions match expected profilesThe new loop items for cloning
ovos-dockervshivemind-dockerwith:
ovos-dockerwhenovos_installer_profile != 'satellite' or ovos_installer_cleaning | boolhivemind-dockerwhenovos_installer_profile == 'satellite' or ovos_installer_cleaning | booland
when: item.state | boolgive the right behavior: only the relevant repo is cloned for normal installs, and both are available when cleaning. This looks correct.ansible/roles/ovos_hardware_mark1/handlers/main.yml (1)
11-12: Expose role-specific reboot fact alongside legacy variable.The addition of
ovos_hardware_mark1_rebootfollows proper Ansible naming conventions, while retainingovos_installer_rebootwith a noqa comment for backward compatibility. This dual approach is appropriate for a phased migration.ansible/roles/ovos_hardware_mark1/tasks/prepare.yml (2)
2-6: Task name improved and group append mode added.The task name is now clearer, and
append: truecorrectly prevents group removal during user modification. Changes are appropriate.
19-25: Copy task properly configured with ownership and permissions.Permissions and ownership are now explicitly set for the initialize.sh script. Verify that
_ovos_hardware_mark1_workind_directoryis defined in role defaults or variables (note: potential typo—"workind" vs. "working").ansible/roles/ovos_hardware_mark2/tasks/prepare.yml (1)
17-21: Boot-directory variable consistently renamed with proper conditional.Variables
ovos_hardware_mark2_boot_config_statusandovos_hardware_mark2_boot_directoryfollow the role-prefixed naming convention and replace underscore-prefixed private variables. The conditional correctly references the new status variable.ansible/roles/ovos_hardware_mark2/handlers/main.yml (1)
17-18: Role-specific reboot fact added with legacy variable retained.Parallels the mark1 handler pattern:
ovos_hardware_mark2_reboot(properly named) alongside legacyovos_installer_reboot(with noqa comment). Appropriate for backward compatibility during migration.ansible/roles/ovos_hardware_mark2/tasks/vocalfusion.yml (3)
26-26: Boot-directory variable updated consistently; inline conditional refactored.Line 26 correctly uses
ovos_hardware_mark2_boot_directory, and line 28 inlines the conditional logic previously computed in a vars block. Both changes maintain behavior while improving clarity.Also applies to: 28-28
64-64: Task names simplified for clarity without functional change.Lines 64 and 112 remove variable interpolation from task names, improving readability. No logic changes.
Also applies to: 112-112
75-89: YAML folded scalars verified—URLs parse correctly without newlines.Verification confirms all URLs maintain single-line integrity with the folded scalar syntax (>-):
- xvf3510-flash: 154 chars, single-line ✓
- app_xvf3510_int_spi_boot_v4_2_0.bin: 176 chars, single-line ✓
- init_tas5806: 95 chars, single-line ✓
Loop structure and destinations are correct.
ansible/roles/ovos_hardware_mark2/tasks/firmware.yml (1)
17-26: EEPROM-related variables consistently renamed with proper conditional evaluation.Variables
ovos_hardware_mark2_eeprom_statusandovos_hardware_mark2_eeprom_rebootreplace underscore-prefixed private variables. All references—including the when condition at lines 25-26—correctly use the new names. The dual set_fact pattern (legacyovos_installer_reboot+ properovos_hardware_mark2_reboot) maintains backward compatibility.ansible/roles/ovos_hardware_mark2/meta/main.yml (1)
28-29: Collectionmoreati.uvis available on Ansible Galaxy; verify Ansible version compatibility.The moreati.uv collection is published to Ansible Galaxy with latest version 0.0.2 (released Dec 4, 2024). The role declares
min_ansible_version: "2.12". Before merging, verify that moreati.uv v0.0.2 is compatible with your target Ansible version (≥2.12) and test themoreati.uv.pipmodule usage in your environment.
tests/molecule/ovos_installer/roles/test_ovos_installer/tasks/configuration.yml
Show resolved
Hide resolved
goldyfruit
added
enhancement
1 participant
Summary by CodeRabbit
Bug Fixes
New Features
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.