Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor rubies install and remove tasks #219

Merged
merged 5 commits into from
Aug 18, 2020

Conversation

danochoa
Copy link
Contributor

I think this should help clean up a bit and resolves #218.

@danochoa danochoa requested a review from a team as a code owner July 31, 2020 07:49
@thbar
Copy link
Contributor

thbar commented Aug 1, 2020

@danochoa I will be able to test this on Monday. I will get back to you!

Copy link
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 believe fetching the list of installed rubies and using a difference will lead to something both faster and less potentially problematic!

Just my opinion though, if other input can be made from other maintainers, this is welcome.

tasks/rubies.yml Show resolved Hide resolved
failed_when: False
register: detect_rubies
with_items: '{{ rvm1_rubies }}'
when: rvm1_rubies is defined and rvm1_rubies | length > 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can generally avoid checking if variables are defined in specific cases where the variable is defined in defaults/main.yml. Just my thoughts. If code owners agree, I will probably open a separate PR to clean this up throughout the other tasks. Othewise, I'm happy to continue always checking if a variable is defined before using it.

changed_when: False
register: rvm1_list_installed_rubies
when: rvm1_delete_ruby is defined and rvm1_delete_ruby != ''

- name: Delete ruby if relevant
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With rvm1_delete_ruby as a string (single ruby), I think we can leave this alone for now. If this becomes a list we can use the difference from rvm_list_strings similar to the install task.

@danochoa danochoa requested review from thbar and a team August 4, 2020 17:05
@danochoa
Copy link
Contributor Author

danochoa commented Aug 6, 2020

@thbar this is ready for another review when you get a chance. Thanks.

@thbar
Copy link
Contributor

thbar commented Aug 6, 2020

@danochoa I will only be able to check this out in ~2 weeks (but will do that if nobody has done it before).

Poke @rvm/ansible if someone else is more available right now!

@danochoa
Copy link
Contributor Author

danochoa commented Aug 6, 2020

@danochoa I will only be able to check this out in ~2 weeks (but will do that if nobody has done it before).

Poke @rvm/ansible if someone else is more available right now!

@thbar Thanks for the update.

Copy link
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 have done a bit of testing with some vagrant machines (adding / deleting / idempotence checks), and everything except --check seems to work fine.

I believe it is good enough for now.

command: '{{ rvm1_rvm }} {{ item }} do true'
- name: Detect rubies
command: '{{ rvm1_rvm }} list strings'
register: rvm_list_strings
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this point fails with --check enabled. I don't think it is a huge deal at this point though, because other parts will probably also break or output incorrect stuff (e.g. no changes where changes should be reported), so I think we can leave with that for now.

@thbar
Copy link
Contributor

thbar commented Aug 18, 2020

@rvm/ansible can anyone have a second look? On my end I believe this can be merged, and we will improve things with the next PRs.

@pkuczynski pkuczynski merged commit e332efc into rvm:master Aug 18, 2020
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.

Ansible --check mode fails on "Delete ruby if relevant"
3 participants