Add support for multiple environments

[lmfaber]

Inspired by issue #5, I've implemented a way to support multiple environments with different channels and packages. This is merely a suggestion, please feel free to add any other ideas or issues that I might have overseen.

Cheers.

[maresb]

Hey, thanks so much @lmfaber for the contribution, this looks really cool!

I'm not using Ansible these days, so it's difficult for me to give a proper review, but perhaps @gdevenyi could help?

A few things I noticed:

1. The first commit adds executable permissions to a bunch of files. We should undo that. Also the newline at the end of defaults/main.yml and meta/main.yml has been removed, and we should undo that as well.
2. A preexisting issue I noticed: Python dependencies are specified in the form python=3.9. That's ambiguous because the intent might be either 3.9.0 or 3.9.*. Nowadays the preferred form is 3.9.*, so we should probably update the examples accordingly.
3. It looks like we're switching from micromamba shell hook ... to {{ dest }} shell hook .... I believe the difference is that the first one relies on finding micromamba in the PATH, while the second is an absolute path. If someone already has the former version in their .bashrc then I think we won't detect it and we'll create a second line. But maybe this doesn't matter much?
4. This PR refactors the way the micromamba command is constructed. The previous version carefully handled unspecified arguments, and with these changes it's unclear to me how robust the new version is. (I haven't actually tested it myself.)

Thanks again @lmfaber!

[lmfaber]

Hi @maresb,

thanks a lot for your fast response and your input!

1. You're absolutely right, there should not be executable permissions on those files. I've removed it. I also added the new lines that were deleted.
2. I've adjusted this as well.
3. You're right, it might create a second entry in the .bashrc, which might be problematic, depending on the use case. I've decided to use the {{ dest }}, as this reflects the final installation location for the micromamba executable. As this will be evaluated in the .bashrc, there's no need for the installation directory to be in the PATH. However, I'm open to change this back again. I'm new to Ansible scripts and might not know the best practices here and there.
4. I agree, I don't know how robust my method of creating the environments is. It's working for the example I provided, but there might be more cases that should be tested. Is there maybe a test set available that I can test is against?

Cheers.

[maresb]

Thanks a lot for cleaning up the permissions and deleted EOLs, the diff for this PR is much cleaner now!

I agree that {{ dest }} is better and more robust. My only concern is migrating existing users in a clean way. One potential idea is to have some conditional logic to check for the old micromamba line and replace it accordingly, but I'm concerned that could introduce a lot of complexity.

Regarding the new method for creating environments, is there a particular reason you rewrote the logic?

I haven't used Ansible for a few years now, so I'm really hoping that @gdevenyi can help with testing, current best practices, etc.

[lmfaber]

Great, I will leave {{ dest }} for now then.

Absolutely, we should make the migrating as easy as possible for users. I've had a look at replacing the old micromamba line, which should be possible by doing something along the lines (not tested yet):

- name: Check if alternative micromamba line exists
  ansible.builtin.shell: grep -qE 'eval "\$\(.*/micromamba shell hook --shell=bash\)"' /etc/bashrc
  register: micromamba_alt_line_check
  changed_when: false
  failed_when: false

- name: Remove default micromamba eval line
  ansible.builtin.lineinfile:
    path: /etc/bashrc
    regexp: '^eval "\$\(micromamba shell hook --shell=bash\)"$'
    state: absent
  when: micromamba_alt_line_check.rc == 0

Of course, this need to be done for all the different OS. To keep it a bit more organized, my suggestion would be to separate all the bashrc steps from the tasks/main.yml into tasks/setup-bashrc.yml. This would include checking for the old micromamba then. What do you think?

I mailny rewrote the logic, because I needed the for loop for the different environments to be created and I didn't know how to incorporate the current three steps (Build first part of arguments, Build channel part of arguments, Create a new conda environment) into that loop properly otherwise. I'm really open to suggestions here, as I'm mainly limited by my missing Ansible knowledge 😄

[maresb]

Makes sense to me. I've reviewed this as thoroughly as I practically can without relearning Ansible.

I wonder if @atrawog is still using Ansible? He might be able to help as well.

[lmfaber]

I've added the changes regarding the bashrc files (separating in a new file and deleting the old micromamba statement if present). That should help migrating users. It's not the cleanest solution. I'm pretty sure one could use a loop here as well, but I decided against if for now.

The changes were tested with a bashrc file that contains the old statement and no new one:

eval "$(micromamba shell hook --shell=bash)"

The Ansible output is then the following:

TASK [mambaorg.micromamba : Enable micromamba in bash (redhat-like)] ************************************************************************************************************************************************************************************************************************************************
--- before: /etc/bashrc (content)
+++ after: /etc/bashrc (content)
@@ -89,3 +89,4 @@
 # vim:ts=4:sw=4
 
 eval "$(micromamba shell hook --shell=bash)"
+eval "$(/test/path/micromamba shell hook --shell=bash)"

changed: [w190-galaxy2]

TASK [mambaorg.micromamba : Replace old micromamba line if present (redhat-like)] ***********************************************************************************************************************************************************************************************************************************
--- before: /etc/bashrc (content)
+++ after: /etc/bashrc (content)
@@ -88,5 +88,4 @@
 fi
 # vim:ts=4:sw=4
 
-eval "$(micromamba shell hook --shell=bash)"
 eval "$(/test/path/micromamba shell hook --shell=bash)"

changed: [w190-galaxy2]

Regarding the refactoring of the env creation logic, I'd suggest we wait a bit for @atrawog to respond as I'm not in a hurry with those changes. If you decide you'd like to keep the old logic, let me know and I'll have a look again. Thanks for your help @maresb

Cheers.

[gdevenyi]

Hi.

All my code was written assuming that micromamba was going to end up in a system-wide PATH that users have accessible. I think if there's any deviation from the default install that we can't safely assume that the path will be accessible via all users and things shouldn't be configured in a system-wide way. I don't think I'd want to see the path to the micromamba binary hard-coded into the scripts either.

Perhaps the better solution here is to return to my implementation and wrap it in an option that's off by default.

For user level installs, it should probably do something more like:
https://github.com/mamba-org/ansible-role-micromamba#with-an-initialization-script
after the installation.

[maresb]

All my code was written assuming that micromamba was going to end up in a system-wide PATH that users have accessible. I think if there's any deviation from the default install that we can't safely assume that the path will be accessible via all users and things shouldn't be configured in a system-wide way. I don't think I'd want to see the path to the micromamba binary hard-coded into the scripts either.

You make a really good point. The scenario I had in mind to protect against is that you may have multiple versions of micromamba available, and it can be confusing if you initialize the wrong one, and so hard-coding would protect against this. On the other hand, unless there's a misconfiguration then any user should have their particular micromamba they intend to use specified in PATH (ignoring order-of-operations in .bashrc 😣).