Skip to content

Conversation

Valkyrie00
Copy link
Contributor

This PR replaces hardcoded 'zuul' user and group references throughout the CI-Framework codebase with configurable variables to improve flexibility and follow the project's convention of using parameterized variables.

1. libvirt_manager Role

  • Files modified: roles/libvirt_manager/tasks/manage_vms.yml, roles/libvirt_manager/defaults/main.yml
  • Changes:
    • Replaced hardcoded 'zuul' with {{ cifmw_libvirt_manager_user }}
    • Added configurable variable: cifmw_libvirt_manager_user: "{{ ansible_user | default(lookup('env', 'USER')) }}"

2. Infrastructure Playbook

  • File modified: create-infra.yml
  • Changes:
    • Replaced hardcoded 'zuul' with {{ ansible_user_id }}

3. Reproducer Role

  • Files modified: roles/reproducer/tasks/configure_controller.yml, roles/reproducer/tasks/ci_job.yml, roles/reproducer/defaults/main.yml
  • Changes:
    • Replaced hardcoded 'zuul' with {{ cifmw_reproducer_user }}
    • Added configurable variable: cifmw_reproducer_user: "{{ ansible_user | default('zuul') }}"

4. Hook Playbooks

  • File modified: hooks/playbooks/delete_all_pre_adoption_resources.yaml
  • Changes:
    • Replaced hardcoded 'zuul' with {{ ansible_user | default('zuul') }}
  • Impact: Adoption cleanup operations work with different user contexts

Backward Compatibility

  • Variable precedence: Uses appropriate Ansible facts (ansible_user_id, ansible_user)
    based on context
  • Backward compatibility: Maintains 'zuul' as fallback where appropriate

@Valkyrie00 Valkyrie00 self-assigned this Sep 1, 2025
@Valkyrie00 Valkyrie00 requested a review from a team as a code owner September 1, 2025 17:37
Copy link
Contributor

openshift-ci bot commented Sep 1, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

amartyasinha
amartyasinha previously approved these changes Sep 2, 2025
@@ -22,8 +22,8 @@
- name: Create openstack config dir
ansible.builtin.file:
path: "{{ ansible_user_dir }}/.config/openstack/"
owner: zuul
group: zuul
owner: "{{ ansible_user_id }}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, ansible_user_id is the name of the user which is executing the playbook, where ansible_user is the username on which you ssh to the remote host, right?
Wondering if ansible_user would be not enough here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly right! ansible_user_id is the actual executing user, ansible_user is the SSH username.
Here in particular I used ansible_user_id because:

  • We're creating local files ({{ ansible_user_dir }}/.config/openstack/)
  • We need them owned by the real system user, not the SSH user
  • With gather_facts: true, ansible_user_id gives us the actual executing user

I think that ansible_user could create a mismatch where files are in /home/idk_user/ but owned by ssh_user, causing permission issues, so this is why I choose ansible_user_id. But I want to be sure, so please let me know if my considerations are incorrect. Any doubts are really appreciated! 🙏

In general, I have tried to use ansible_user_id when gather_facts is true and it is something closely related to files/directories.

Copy link

@yorabl yorabl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some references to zuul user in roles/reproducer/tasks/configure_controller.yml that should be changed

@@ -6,8 +6,8 @@
- name: Create openstack config dir
ansible.builtin.file:
path: "/home/zuul/.config/openstack/"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The path still uses the user zuul

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @yorabl, you're absolutely right! The title of my PR is probably not entirely accurate in relation to the scope of the activity and creates some confusion. I've updated it.
I also spoke to @danpawlik about this issue, because it appears in several parts of the project.

So I was thinking of creating a specific task (probably more than one) to solve the problem of replacing all hardcoded zuul, even for the part where it can be replaced with something like {{ ansible_user_dir }} for the path and not just for the owner/group, which is the scope of this PR.

What do you think?
But to move forward, I have already updated these two values you mentioned. Thanks! 🙏

@@ -276,8 +276,8 @@
ansible.builtin.copy:
dest: "/home/zuul/.ssh/devscripts_key"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Path have zuul hardcoded

@Valkyrie00 Valkyrie00 changed the title feat: replace hardcoded 'zuul' user with configurable variables feat: replace hardcoded 'zuul' for the owner/group with configurable variables Sep 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants