Add rvm1_delete_unmanaged_rubies option (and deprecate rvm1_delete_ruby)

[gildegoma]

This new feature came to my mind after following discussion.

[gildegoma]

Not sure why the Travis CI build wasn't started... There is probably a syntax error in .travis.yml (I don't think this is related to the "Draft" state of the pull request)

---

Edit:

Indeed, the '{"rvm1_rubies" : ["ruby-2.5.7"]}' json payload is breaking the YAML-to-JSON processing on Travis side 😒.

[thbar]

A few quick thoughts (I'm on the move):

• I wonder if we should keep rvm1_delete_ruby soft-deprecated (still supported for one version). Maybe it's overboard though. Maybe just bumping up the version is enough (I would be fine with this!)
• We should have a reliable support for --check mode, because if any trouble occurs in the detection logic (e.g. versions detection due to a change in output of RVM itself), there could probably be a risk of deletion of all the rubies by mistake.
• Alternatively, a "less clever" approach using just an explicit, user-provided list of rubies to be deleted (just like rvm1_delete_ruby but with a manually handled list (via a variable), could be safer.

Again raw thoughts, I'm on the move, but thanks for taking a stab at this, I'll let other comment!

[danochoa]

I think its a good idea and I think I prefer the rvm_delete_unmanaged_rubies over an explicit rvm_delete_rubies list. I'd prefer not to maintain separate install and delete lists. But I'm not against the list method if it seems safer, although not sure if that's true.

One question I have (not having a lot of experience with rvm outside of minimal use of this role), since I think rvm will not be able to delete system (non-rvm installed) rubies, should we add something to explain this? To avoid confusion with the unmanaged term.

[danochoa]

I think loop is now preferred over with_items.
https://docs.ansible.com/ansible/latest/user_guide/playbooks_loops.html

And maybe use rvm1_list_installed_rubies.stdout_lines instead of rvm1_list_installed_rubies.stdout.splitlines().

[gildegoma]

I copy-pasted rvm1_list_installed_rubies.stdout.splitlines() from the existing tasks, but thank you for giving this hint. I'll change it in both tasks.

About loop versus with_items: Since all the "looping" tasks in this role are still using the original with_items style, I didn't want to mix both syntaxes. I won't change it for the moment, and would propose to convert all the relevant tasks via a single ad-hoc commit.

[gildegoma]

See fada459

[gildegoma]

@thbar @danochoa Thank you very much for all your inputs 💓

With fada459, I think that I've already fixed a good part of the requests that you raised.
⚠️ For the moment, I won't add more changes to this PR until we get cleared if the feature idea is accepted, or not 😉.

Preamble/Disclaimer (that I should have posted in the PR description 🙈):

1. In the era of Immutable Infrastructure, I usually won't have to care much about removing stuff (as I usually provision new images for server deployments). But if I have to deal with an existing state, I'd prefer not having to be explicit on every things that shouldn't exist (if possible).
2. When performing "in-place" updates, I think that it doesn't hurt much (if you have disk space) to leave in place the rubies previously installed by RVM. This is actually the default and safe behaviour of this role.
3. As @thbar said, if the check mode is working, it is possible to verify with a "dry run" which versions would be removed when rvm1_delete_unmanaged_rubies is enabled.

Now here my comments/answers to the following points:

I wonder if we should keep rvm1_delete_ruby soft-deprecated. [...] Maybe just bumping up the version is enough (I would be fine with this!)

@pkuczynski What are your thoughts about that point? (Backwards compatibility, Semantic versioning,...)

We should have a reliable support for --check mode, because if any trouble occurs in the detection logic (e.g. versions detection due to a change in output of RVM itself), there could probably be a risk of deletion of all the rubies by mistake.

The check mode is now fixed (fada459) and I doubt that rvm list strings output may change much in the future.

[...] should we add something to explain this? To avoid confusion with the unmanaged term.

The variable name rvm1_delete_unmanaged_rubies is just a first proposition, but I'm open to any naming that could be more self-explanatory. And same for the README documentation. I just think, that the Ansible Role documentation can assume that the user knows RVM scope and capabilities.

Thank you again for your help on this 😃

[pkuczynski]

I think dropping a function, bumping the version, and mentioning in the changelog's section Breaking Changes, should be enough if we feel that maintaining something does not make any sense or goes against the new direction of the package.

[thbar]

@gildegoma I will be able to test beginning of next week. Will comment back unless someone approves before!

[thbar]

@gildegoma can't test this for now actually, but will have a bit more time next month. I'll report back if/when I can.

[gildegoma]

FYI @thbar: I'd keep this PR in Draft state, until #220 is merged.

[pkuczynski]

@sfgeorge @danochoa @thbar what do you think?

[sfgeorge]

I'm a little torn on this one.

On the one hand, it is very nice to be Declarative and to be able to say "These Rubies should exist, no more, no less. Don't care what was there before – Let's clean it up to be in this state now.".

However, this can have unexpected consequence for some workflows. I would not be surprised if someone deployed RVM+Ruby to existing servers in their organization using a brand new playbook (and not knowing that someone else has installed RVM + a Ruby version already). The removal would certainly cause some applications to break.

I am thinking about how different the proposed behavior is to this convention in Ansible:

- name: Install the latest version of Apache and MariaDB
  ansible.builtin.package:
    name:
      - httpd
      - mariadb-server

...By saying I want these 2 packages, I'm not implying that Ansible should remove All of the other packages on the system not mentioned here. I know this is an unfair comparison, but I'm trying to thinking of any precedent for this sort of convention in Ansible and I can't think of one.

[thbar]

However, this can have unexpected consequence for some workflows.

I also have the same feeling. Removing everything that is not explicitly specified, while nice in theory, could end up with troubles.

Worst case it is easy for someone to create a task to issue multiple deletes, and over 5 years of Ruby maintenance I mostly have to delete one or two rubies at a time, to N calls to rvm1_delete_ruby have been good enough for me.

[pkuczynski]

Lets then close it in favor of #221