search

rvm / rvm1-ansible

The official ansible RVM role to install and manage your Ruby versions.
MIT License
271 stars 136 forks source link

Add rvm1_delete_unmanaged_rubies option (and deprecate rvm1_delete_ruby) #222

Closed gildegoma closed 2 years ago

gildegoma commented 4 years ago

This new feature came to my mind after following discussion.

gildegoma commented 4 years ago

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)

image

Edit:

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

thbar commented 4 years ago

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

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

gildegoma commented 4 years ago

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

With fada459a67fc67e0d5a11dee507d2ab76ecc7a3a, 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 (fada459a67fc67e0d5a11dee507d2ab76ecc7a3a) 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 commented 4 years ago

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 commented 4 years ago

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

thbar commented 4 years ago

@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 commented 4 years ago

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

pkuczynski commented 2 years ago

@sfgeorge @danochoa @thbar what do you think?

sfgeorge commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Lets then close it in favor of #221