Skip to content

Tests: Verify that 'rvm1_delete' functionality is honoured#221

Merged
pkuczynski merged 3 commits into
rvm:masterfrom
gildegoma:test-delete-function
Mar 26, 2022
Merged

Tests: Verify that 'rvm1_delete' functionality is honoured#221
pkuczynski merged 3 commits into
rvm:masterfrom
gildegoma:test-delete-function

Conversation

@gildegoma
Copy link
Copy Markdown
Contributor

This is a first move to try to "fully cover" the features offered by this role.

It is maybe time to consider if this project should continue to implement its specs via Ansible assertions, or adopt some new framework (such as Molecule, TestInfra, ServerSpec, ...).

Bonus Question: Do we really want to keep such feature? If yes, maybe this should deal with a list of rubies... The question is motivated by ongoing debates about Bundler setup being a duty, or not, of this role.

@gildegoma gildegoma requested a review from a team as a code owner August 2, 2020 16:02
Copy link
Copy Markdown
Contributor

@thbar thbar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggested a change (see associated comment), but I also believe the PR as it is is a great improvement already.

I am familiar with ServerSpec (not yet with Molecule, I believe they will work together afaik), and indeed if the test suite grows it could be interesting to have that in place, good thought!

Comment thread tests/assertions.yml
@thbar
Copy link
Copy Markdown
Contributor

thbar commented Aug 4, 2020

A comment on this by the way:

Bonus Question: Do we really want to keep such feature? If yes, maybe this should deal with a list of rubies... The question is motivated by ongoing debates about Bundler setup being a duty, or not, of this role.

I would personally like to keep this feature in the role, because otherwise I'll have to SSH into my machines.

I do agree though that it should be able to deal with a list of rubies, work idempotently, so that I can keep the list of old rubies in there for some times (particularly useful if I wake up a slightly old vagrant VM which isn't up-to-date ; the old "delete" list will automatically clean up what's been moved to delete).

@Kulgar
Copy link
Copy Markdown

Kulgar commented Aug 4, 2020

I would personally like to keep this feature in the role, because otherwise I'll have to SSH into my machines.

I kinda agree with Thibaut on this one.
Regarding to me, rvm was created to manage rubies (it is named ruby version manager after all).

So, everything that helps people to manage rubies (including removing old rubies), is good to be added to this role I think.

But... rvm wasn't meant to deal with gems installation, gems are handled by rubygems (which is installed by rvm). So, I think bundler (and any other gems) shouldn't be managed by this role as it would add too much complexity as we already faced with bundler.

I don't know if I'm the only one to think that way? :-)

@danochoa
Copy link
Copy Markdown
Contributor

danochoa commented Aug 4, 2020

@gildegoma good job on the PR, it seemed like this was missing from the tests. I don't have any experience with the test frameworks so I can't comment on that, sorry.

I agree, I think rvm1_delete_ruby would be more useful as a list, but I think that could be a breaking change, not sure breaking changes are usually managed here.

@gildegoma
Copy link
Copy Markdown
Contributor Author

gildegoma commented Aug 5, 2020

About improving (or evolving from) rvm1_delete_ruby, I propose the following approach: #222.

@pkuczynski
Copy link
Copy Markdown
Member

@sfgeorge @danochoa @thbar is this ready for merge? Let me know and I will merge it...

Copy link
Copy Markdown
Contributor

@sfgeorge sfgeorge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm in favor of this 👍

It further covers testing, while acknowledging that more could be covered once we switch to a more robust testing methodology.

That switch to another testing framework shouldn't hold-up the fact that this is certainly incrementally better than what we had before. I like it. 👍

@thbar
Copy link
Copy Markdown
Contributor

thbar commented Mar 26, 2022

@pkuczynski I am in favour of this too ; this is a quality improvement on an existing feature that I still use regularly, so definitely happy to see this.

I feel safer to delete one by one with this, compared to #222.

@pkuczynski
Copy link
Copy Markdown
Member

Perfect. Waiting for build to pass and merging in! :)

@pkuczynski pkuczynski merged commit afa5031 into rvm:master Mar 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants