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

Use native modules for bundler tasks #402

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Matt-Yorkley
Copy link
Collaborator

Removes a bit more dependence on bash scripts and bash -lc ... in favour of native Ansible modules.

🔥🔥

@Matt-Yorkley Matt-Yorkley self-assigned this Apr 16, 2019
Copy link
Member

@mkllnk mkllnk 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 not convinced that replacing one shell task with three others is an improvement. But I'm also biased because I wrote that shell script. ;-) When I saw the 🔥 signs in the PR description I was hoping for less code.

I agree that native Ansible modules integrate better into our playbooks. But I'm concerned about the code duplication of grep -m 1 -A 1 -x -F "BUNDLED WITH" Gemfile.lock | tail -n 1 | tr -d '[:space:]'. I don't want to get rid of the shell script because it's useful in dev environments. It's hardly a big deal though. 🤷‍♂️

@Matt-Yorkley
Copy link
Collaborator Author

Matt-Yorkley commented Apr 17, 2019

I'm not convinced that replacing one shell task with three others is an improvement. But I'm also biased because I wrote that shell script. ;-) When I saw the fire signs in the PR description I was hoping for less code.

I was mostly trying to see where we could use more native modules and less bash -lc ... in our playbooks. The original script is still useful in dev. 👍

grep -m 1 -A 1 -x -F "BUNDLED WITH" Gemfile.lock | tail -n 1 | tr -d '[:space:]' is a fantastic line! We couldn't do without it, but we can use it with register and changed_when: False, and then handle the real tasks of gem bundler install ... and bundle install with the gem and bundler modules.

@Matt-Yorkley Matt-Yorkley force-pushed the native_modules branch 2 times, most recently from 693e689 to 15d9c88 Compare April 22, 2019 18:06
@Matt-Yorkley
Copy link
Collaborator Author

I've retested this and there's some issues, possibly with rbenv's rehash system. Lets leave it for now.

@sauloperez
Copy link
Contributor

Agree. This is post v2. We have more urgent matters for that release to be rolled out smoothly.

@luisramos0
Copy link
Contributor

moving to in dev.

@mkllnk
Copy link
Member

mkllnk commented Feb 25, 2020

Is it a good time to pick this up again, @Matt-Yorkley? Or should we close it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: All the things 💤
Development

Successfully merging this pull request may close these issues.

5 participants