Multiple sites on the same hosts enhancements

[mamedin]

This PR adds:

• Use a different revision directory for every site update.
• Add new tasks file for CLI commands.
• Add private symlink.
• Add downloads symlink.
• Ensure downloads and uploads symlink directories exist.

Connects to #53 and #20

[amayita]

Looks awesome!

[hakamine]

I'll try to summarize my added comments below:

1. Example in README.md needs quick correction (either change /usr/share/nginx/atom in the first line with /use/share/nginx/test5, or change the "test5" in the other lines with "atom")
2. Try to avoid defining atom_extra_path in defaults/main.yml (as it is not a variable whose value could be customized by the user)
3. task/cli_tools.yml is adding tasks that build the AtoM themes, but there are tasks that already do a similar thing in tasks/build.yml
4. git task added in tasks/revision-dir.yml could fail if the repository has not been already cloned before?
5. Is the "private" symlink needed?

For 2. and 4. I was thinking we could for example:

• In defaults/main.yml remove the atom_extra_path:"" declaration
• In tasks/main.yml, remove the when conditional for task include: "revision-dir.yml"
• Refactor revision_dir.yml for example like (haven't tested it):

- name: Get latest revision
  shell: "git ls-remote {{ atom_repository_url }} {{ atom_repository_version }} | awk '{print $1}'"
  register: latest_revision
- name: "Define atom_extra_path when using atom_revision_directory"
  set_fact:
    atom_extra_path: "atom-{{ latest_revision.stdout }}"
  when: 
atom_revision_directory is defined and atom_revision_directory|bool 
- name: "Define atom_extra_path as empty when not using atom_revision_directory"
  set_fact:
    atom_extra_path: ""
  when: 
atom_revision_directory is undefined or not atom_revision_directory|bool

[mamedin]

Thanks very much for CR!

I have changed the branch to include your suggestions. Using the summary points added by @hakamine in last comment:

0. Fixed duplicated variables added to defaults/main.yml

1. Fixed example in README.md

2. atom_extra_path is now defined with set_fact. I added this task in tasks/main.yml because this variable is used by drmc-mock and atom-devbox tasks. The "always" tag has been added to this task and the include: "revision-dir.yml" one because it is used by almost all tags, the explicit list should be:

   • "atom-site"
   • "drmc-mock"
   • "atom-devbox"
   • "atom-basic"
   • "atom-flush"
   • "atom-plugins"
   • "atom-search"
   • "atom-cli"
   • "atom-build"
   • "atom-worker"
   • "atom-uploads"
   • "atom-downloads"

3. (No changes from last PR branch) The "Compile all themes" task is useful to avoid having to explicitly declare the themes you want to build in the atom_themes dictionary. Do you think it is a good idea to add and additional conditional to the existing "Build AtoM themes (DISTRO)" tasks (to avoid compile 2 times the themes when atom_compile_all_themes|bool)? For instance:

   • not atom_compile_all_themes|bool

4. I have not detected any issue when the repository has not been already cloned before.

5. Probably the "private symlink" task doesn't make sense in the AtoM role. We can remove the "private symlink" commit and create the post_task directly in the AtoM playbook when needed.

6. Added a new commit to use version() instead of version_compare() <- Python3 compatibility

[hakamine]

@mamedin thank you for the fixes. I have a comment regarding the use of the "always" tag in role, the tasks will execute every time a playbook is run and there is no way to exclude them. For example, the following playbook:

- hosts: somehost
  roles:
    - role: "nginx"
      tags: nginx
    - role: "atom"
      tags: atom

even if running the playbook with tag nginx (e.g. ansible-playbook playbook.yml -t nginx) it will execute the tasks in the atom role tagged with "always". It's probably safe if the tasks do not modify the target host (assigning vars, etc), but it could be kind of confusing.

[DavidHume-ArtefactualSystems]

@mamedin @hakamine - I believe for atom_revision_directory style deploys of AtoM the private symlink is required for digital object access control (copyright etc.), at least was when originally set up, I'd have to recall which Hosted clients definitely used it (Mills?) and per my using this method for SFU this year, should be required.

Or Miguel are you saying it would still be present if the boolean is set, just not going to do it in the role now (number 5. in your lists).

[mamedin]

@mamedin @hakamine - I believe for atom_revision_directory style deploys of AtoM the private symlink is required for digital object access control (copyright etc.), at least was when originally set up, I'd have to recall which Hosted clients definitely used it (Mills?) and per my using this method for SFU this year, should be required.

Or Miguel are you saying it would still be present if the boolean is set, just not going to do it in the role now (number 5. in your lists).

I was thinking on moving the private symlink task to our atom26.yml playbook as a post_task.

[mamedin]

Ups, wrong click!

[mamedin]

@mamedin thank you for the fixes. I have a comment regarding the use of the "always" tag in role, the tasks will execute every time a playbook is run and there is no way to exclude them. For example, the following playbook:

- hosts: somehost
  roles:
    - role: "nginx"
      tags: nginx
    - role: "atom"
      tags: atom

even if running the playbook with tag nginx (e.g. ansible-playbook playbook.yml -t nginx) it will execute the tasks in the atom role tagged with "always". It's probably safe if the tasks do not modify the target host (assigning vars, etc), but it could be kind of confusing.

I'm going to change the always tag to the explicit list. Thanks Hector!

[mamedin]

Hi!, branch updated again. Using again the summary points, these are the new changes:

2.- "always" tag changed on revision tasks, the tasks are using the big tag list instead.
3.- Build all themes tasks have been moved to the build.yml task file.
7.- New commit. The "deph" and "force" options for git clone are now configurable. Using old values as default options.

[DavidHume-ArtefactualSystems]

(Cosmetic) Having moved this section back to tasks/build.yml, should remove this comment line.

[mamedin]

Good catch!

[DavidHume-ArtefactualSystems]

In my tests, even with a previous revision these two results would be the same - but I think that might be because of the other issues we encountered. Hence my wondering @mamedin if you have gotten this output with different results yet. That is, atom_revision_git_output.before value via akin to what we used to keep in the latest file before a branch update, and atom_revision_git_output.after being the true current (which I assume the above first task does more efficiently than shell command "git ls-remote {{ atom_repository_url }} {{ atom_repository_version }} | awk '{print $1}'" did).

That said, I think as we discussed as long as .after is correct as used in the "Define atom_extra_path..." task below (so won't try to overwrite a previous or fallback revision directory) all is well...

[DavidHume-ArtefactualSystems]

@hakamine - did you have a similar concern, I guess this relates to your refactor suggestion for revision-dir.yml on August 10 above?

[hakamine]

@DavidHume-ArtefactualSystems when I first saw the PR code I was concerned that maybe using git with "clone: no" and "update:no" would fail if there was no local repo. But re-checking the ansible docs, they provide a usage example of git with "clone: no" and "update:no", to "just get information about the repository whether or not it has already been cloned locally." therefore I think it should be good regarding my original concern. However, the problem you encountered seems to be a different issue.

[mamedin]

Probably related to:

https://github.com/artefactual-labs/ansible-atom/blob/master/tasks/basic.yml#L3-L9

Using depth=1 is known problem.

[luisilara]

I tested the new changes on a client upgrade and it worked properly

• The revision-dir.yml improves the command shell: "git ls-remote {{ atom_repository_url }} {{ atom_repository_version }} | awk '{print $1}'" where in some cases it created the revision folder with the stdout with two lines eg. 'atom-d6ffd39c150ebe74adb0bc9e7e8d84797ad8def2'$'\n''c017acb60e8dceeb6ec453e368381bf12bc940ad'/
• The new variable atom_git_pull_code_depth will produce a shallow clone, probably the default value could be set to 1 to avoid adding that variable to the playbook?

[mamedin]

I tested the new changes on a client upgrade and it worked properly

• The revision-dir.yml improves the command shell: "git ls-remote {{ atom_repository_url }} {{ atom_repository_version }} | awk '{print $1}'" where in some cases it created the revision folder with the stdout with two lines eg. 'atom-d6ffd39c150ebe74adb0bc9e7e8d84797ad8def2'$'\n''c017acb60e8dceeb6ec453e368381bf12bc940ad'/
• The new variable atom_git_pull_code_depth will produce a shallow clone, probably the default value could be set to 1 to avoid adding that variable to the playbook?

@luisilara The omit variable is an ansible special variable:

https://docs.ansible.com/ansible/latest/user_guide/playbooks_filters.html#omitting-parameters

So the default value is depth: null when running the playbook with -vvv option (the same value that you can see when not defining the depth option in task).

And about setting 1 as default value, there's a comment in role about it:

https://github.com/artefactual-labs/ansible-atom/blob/master/tasks/basic.yml#L3-L9

So I prefer to use the old default values for depth and force and change it with extra-vars when is needed.

[hakamine]

@mamedin I think the accept_hostkey: "yes" parameter may also be needed in the git task in tasks/revision-dir.yml (currently only the git task in tasks/basic.yml has it). I just did a test and failed there.

[hakamine]

@mamedin @DavidHume-ArtefactualSystems What do you think about removing the {{ atom_path }}/{{ atom_extra_path }} prefix in the atom_themes path items in defaults/main.yml (i.e., making the paths in atom_themes relative to the AtoM base directory), and instead adding this prefix to the "Build selected AtoM themes" tasks in tasks/build.yml. I think it could make the atom_themes definitions in host_vars simpler.

[mamedin]

@mamedin @DavidHume-ArtefactualSystems What do you think about removing the {{ atom_path }}/{{ atom_extra_path }} prefix in the atom_themes path items in defaults/main.yml (i.e., making the paths in atom_themes relative to the AtoM base directory), and instead adding this prefix to the "Build selected AtoM themes" tasks in tasks/build.yml. I think it could make the atom_themes definitions in host_vars simpler.

Yes, it will be a nice change, but the problem is that we already have 25 config files with the atom_themes dict in deploy-private repo.

We are going to change some other settings on upgrades, so if you agree I can change it.

[mamedin]

I have upgraded again the branch with these changes:

• The line # Compile available themes in cli_tools.yml has been removed
• Added accept_hostkey: "yes" to the revision task.

[DavidHume-ArtefactualSystems]

@mamedin I think the accept_hostkey: "yes" parameter may also be needed in the git task in tasks/revision-dir.yml (currently only the git task in tasks/basic.yml has it). I just did a test and failed there.

Great catch @hakamine! Thanks @mamedin for latest upgrades.

[hakamine]

@mamedin I ran a test for an upgrade (deployed a branch, then did another deploy with a different code commit; strictly speaking it wasn't a branch update but a branch change), and I think I got the same problem that @DavidHume-ArtefactualSystems reported. The first time I tried the upgrade, the "before:" and "after:" commit hashes reported by the git task were the same (old commit). I had to run the playbook again so that the upgrade was completed, this time the git task reported "before:" and "after:" commit hashes were the same but the new commit (I am NOT using depth=1 BTW). So this meaning that in order to do an upgrade would need to run the playbook twice. Could it be a bug in the ansible git module? Could it be better to stick with the old way (using git ls-remote) ?

[mamedin]

@mamedin I ran a test for an upgrade (deployed a branch, then did another deploy with a different code commit; strictly speaking it wasn't a branch update but a branch change), and I think I got the same problem that @DavidHume-ArtefactualSystems reported. The first time I tried the upgrade, the "before:" and "after:" commit hashes reported by the git task were the same (old commit). I had to run the playbook again so that the upgrade was completed, this time the git task reported "before:" and "after:" commit hashes were the same but the new commit (I am NOT using depth=1 BTW). So this meaning that in order to do an upgrade would need to run the playbook twice. Could it be a bug in the ansible git module? Could it be better to stick with the old way (using git ls-remote) ?

Thanks! I could reproduce the issue when using different branches. I could fix it with git module but using 2 different tasks for getting the current and candidate revision. The dest option is optional when using clone=yes, so I tested without the dest option for the candidate tasks and it fixes the issue.

I prefer to use the git module because the git ls-remote tasks requires remote: 127.0.0.1 and sometimes it fails with AWX or when running the ansible playbook in a different server/VM (ssh forwarding issues).

[mamedin]

Thanks again for suggestions :)

Branch updated again. The revision-dir.yml file has changed:

• Use candidate instead of latest for the revision to be installed. (Cosmetic)
• Use separate tasks to get the candidate and current revisions.