Skip to content
This repository has been archived by the owner on Jan 5, 2023. It is now read-only.

Sync docker context #169

Merged
merged 1 commit into from
Aug 26, 2022
Merged

Conversation

zhan9san
Copy link
Member

Ensure the docker context are copied to work dir.

Related to #168

@zhan9san zhan9san added the bug This issue/PR relates to a bug. label Aug 26, 2022
@zhan9san
Copy link
Member Author

@ssbarnea
Could u kindly review this PR?

@gardar
Copy link
Contributor

gardar commented Oct 10, 2022

@zhan9san This PR introduces rsync as a requirement.
There either needs to be something that triggers rsync install before this step, or ideally the copy module should me used instead of synchronize.

@gardar
Copy link
Contributor

gardar commented Oct 10, 2022

Seems like this feature was present before (#51), and was achieved with the copy module rather than the sync module, I'm not sure when and why it was removed though.

- name: Copy files for image building
copy:
src: "{{ molecule_scenario_directory }}/"
dest: "{{ molecule_ephemeral_directory }}"
mode: "0600"
when: not item.pre_build_image | default(false) and not item.path
loop: "{{ molecule_yml.platforms }}"

@ssbarnea
Copy link
Member

That for multiple reasons, among them:

  • synchronize was moved to ansible.posix, which is not part of ansible-core
  • rsync is not pre-installed on most systems and requires extra tasks for installation

If we are to ever add it back, it would be only based on detection of the required collection and rsync on target host.

Shortly, rsync is fast and cool, but is far less portable than copy.

@zhan9san
Copy link
Member Author

Seems like this feature was present before (#51), and was achieved with the copy module rather than the sync module, I'm not sure when and why it was removed though.

- name: Copy files for image building
copy:
src: "{{ molecule_scenario_directory }}/"
dest: "{{ molecule_ephemeral_directory }}"
mode: "0600"
when: not item.pre_build_image | default(false) and not item.path
loop: "{{ molecule_yml.platforms }}"

This commit you mentioned is in master branch, which is not merged in main branch.

The latest commit id in both branch is 0a5e6c67846b514605892087dacfd8c8d3c46f50.


copy vs synchronize

I think synchronize is better than copy.

We cannot copy all files in "{{ molecule_scenario_directory }}/" , as the molecule.yml would be overwritten. And copy don't have exclude feature.

Instead, we can exclude molecule.yml while using synchronize in #185.

ansible.posix is added in molecule-docker dependency, which will be installed when we run molecule dependency automatically.

@gardar
Copy link
Contributor

gardar commented Oct 11, 2022

I agree that there are limitations in regards of the copy module for this use case and using the synchronize module would be nice, however as it requires extra dependencies to be installed I think it would be better to stick to the copy module. It is not enough to add ansible.posix, you also need to install rsync. And as rsync is a application and not a python module you can't just simply install it with pip, you would need to accommodate different package names and package managers, etc. which would be out of scope for this driver.

The exclude feature is nice, but you can still achieve the same with the copy module by either using a fileglob lookup or by first using find and then filter the list that find returns as you pass it to the copy module.

Something like this (untested - written from memory):

 - name: Copy files for image building 
   copy: 
     src: "{{ item.1 }}"
     dest: "{{ molecule_ephemeral_directory }}" 
     mode: "0600" 
   when: not item.0.pre_build_image | default(false) and not item.0.path 
   loop: "{{ molecule_yml.platforms | product(_molecule_scenario_files_filtered) | list }}"
   vars:
    _molecule_scenario_files_filtered: "{{ lookup('fileglob', molecule_scenario_directory ~ `/*', wantlist=true) | reject('search','molecule.yml') | list }}"

or

- name: Get scenario file list 
   find:
     paths: "{{ molecule_scenario_directory }}"
     patterns: "*"
     recurse: yes
   register: _molecule_scenario_files

 - name: Copy files for image building 
   copy: 
     src: "{{ item.1 }}"
     dest: "{{ molecule_ephemeral_directory }}" 
     mode: "0600" 
   when: not item.0.pre_build_image | default(false) and not item.0.path 
   loop: "{{ molecule_yml.platforms | product(_molecule_scenario_files_filtered) | list }}"
   vars:
    _molecule_scenario_files_filtered: "{{ _molecule_scenario_files | reject('search','molecule.yml') | list }}"

@zhan9san
Copy link
Member Author

zhan9san commented Oct 11, 2022

You almost convince me.

But there has been some existing tools(rsync command and synchronize module) doing this stuff well, we can just use it.

And as rsync is a application and not a python module you can't just simply install it with pip

IMHO, it is not difficult to install with OS package manager.

@gardar
Copy link
Contributor

gardar commented Oct 11, 2022

I'm not arguing that rsync isn't a handy tool and in fact I have it installed on most of my servers/workstations.

However as this driver is being used in stripped down containers in a lot of CI jobs I feel that depending on rsync in this driver to execute a task that can be done without rsync is not ideal. Previously it was enough to just install the driver but now it requires that you manually install rsync in addition.

@ssbarnea
Copy link
Member

You can make it an opt-in feature, but by default we cannot make use of it. Still, we could have a default that uses rsync if module is installed and rsync is also installed on the image.

The idea is to not alter the image during testing without user knowledge as this would nullify the tests for many users.

Good part is that we are not against allowing that, but it needs to be a deliberate decision.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug This issue/PR relates to a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants