Skip to content

CP-13126 Update delphix-platform to install docker from official docker repo instead of docker.io #554

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.

Sign up for GitHub

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

Open
SumoSourabh wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

CP-13126 Update delphix-platform to install docker from official docker repo instead of docker.io #554

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 35 additions & 1 deletion bootstrap/roles/delphix-platform.bootstrap/tasks/main.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,43 @@
name: "{{ item }}"
state: present
with_items:
- docker.io
- nftables
- docker-ce
- docker-ce-cli
- containerd.io
- docker-compose-plugin
Comment on lines +25 to +29
Copy link
Contributor

@sebroy sebroy Jan 27, 2026

Choose a reason for hiding this comment

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

What is the purpose of all of these dependencies?

- python3-docker

- name: Execute 'sudo systemctl start docker' command
ansible.builtin.command: sudo systemctl start docker
register: docker_status_output
changed_when: false # This command only retrieves info, doesn't change state
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

The comment is misleading. The command sudo systemctl start docker does change state by starting the Docker service, so changed_when: false is incorrect. This should either be removed or the task should use proper change detection based on the actual state change.

Suggested change
changed_when: false # This command only retrieves info, doesn't change state

Copilot uses AI. Check for mistakes.
retries: 10
delay: 30
until: docker_status_output.rc == 0
Comment on lines +32 to +38
Copy link
Contributor

@sebroy sebroy Jan 27, 2026

Choose a reason for hiding this comment

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

Can you add a comment explaining why this is needed?

Comment on lines +32 to +38
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

Using sudo within an Ansible command is redundant and potentially problematic. Ansible tasks already run with the privileges defined by become and become_user. Use the ansible.builtin.systemd module instead, which is the idiomatic way to manage systemd services: ansible.builtin.systemd: name=docker state=started enabled=yes.

Suggested change
- name: Execute 'sudo systemctl start docker' command
ansible.builtin.command: sudo systemctl start docker
register: docker_status_output
changed_when: false # This command only retrieves info, doesn't change state
retries: 10
delay: 30
until: docker_status_output.rc == 0
- name: Ensure Docker service is started and enabled
ansible.builtin.systemd:
name: docker
state: started
enabled: yes
register: docker_service_result
retries: 10
delay: 30
until: not docker_service_result.failed

Copilot uses AI. Check for mistakes.

- name: Gather service facts
ansible.builtin.service_facts:
register: services_state

- name: Verify Docker service is running
ansible.builtin.debug:
msg: "Docker service status is {{ services_state.ansible_facts.services['docker.service'].state }}"
when: services_state.ansible_facts.services['docker.service'].state == 'running'
Comment on lines +42 to +47
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

The ansible.builtin.service_facts module automatically populates ansible_facts.services, so the register directive is unnecessary. Access the service facts directly via ansible_facts.services in subsequent tasks.

Suggested change
register: services_state
- name: Verify Docker service is running
ansible.builtin.debug:
msg: "Docker service status is {{ services_state.ansible_facts.services['docker.service'].state }}"
when: services_state.ansible_facts.services['docker.service'].state == 'running'
- name: Verify Docker service is running
ansible.builtin.debug:
msg: "Docker service status is {{ ansible_facts.services['docker.service'].state }}"
when: ansible_facts.services['docker.service'].state == 'running'

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

The when condition makes this verification task only run when Docker is already running, which defeats the purpose of verification. If Docker is not running, the task silently skips. Consider using ansible.builtin.assert to fail the playbook if Docker is not running, or remove the when condition to always report the status.

Suggested change
when: services_state.ansible_facts.services['docker.service'].state == 'running'

Copilot uses AI. Check for mistakes.

- name: Execute 'docker version' command
ansible.builtin.command: docker version
register: docker_version_output
changed_when: false # This command only retrieves info, doesn't change state

- name: Print the full command output
ansible.builtin.debug:
var: docker_version_output.stdout

- name: Print only the Docker client version
ansible.builtin.debug:
msg: "Docker Client Version: {{ docker_version_output.stdout_lines[1] }}"
Comment on lines +40 to +60
Copy link
Contributor

@sebroy sebroy Jan 27, 2026

Choose a reason for hiding this comment

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

What is all of this for?

Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

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

Accessing stdout_lines[1] without bounds checking could cause an index error if the output format changes or is shorter than expected. Add validation to ensure the list has at least 2 elements before accessing index 1, or use a safer extraction method like regex to find the version line.

Suggested change
msg: "Docker Client Version: {{ docker_version_output.stdout_lines[1] }}"
msg: >-
Docker Client Version: {{
(docker_version_output.stdout
| regex_search('Version:\\s*([^\\n]+)', '\\1'))
| default('unknown', true)
}}

Copilot uses AI. Check for mistakes.

- docker_image:
build:
path: "{{ toplevel.stdout }}/docker"
Expand Down
Loading