Refactor bundler settings to use config rs

[a-chacon]

@deivid-rodriguez give me this idea of refactor bundler settings to use config-rs just like rv-settings is doing. I think the code is clearer now.

I just have one question about the precedence, now it is clear:

• Global
• Local
• Environment variables

But before were a logic that confuse me about use_deployment and InstallPath struct. I am not so sure about accomplishing exactly the same logic. But I take care about all specs were working just like before.

[indirect]

I believe the ENV should always override local, and local should always override global. I will wait to hear from @deivid-rodriguez to know if I am right, though. 😅

[a-chacon]

You are right, but yes, let's wait for a review of david!

[deivid-rodriguez]

@a-chacon It's a good question. In my opinion, the precedence that makes the most sense is indeed ENV > local > global, but Bundler weirdly uses local > ENV > global, so that's why the existing logic had that precedence, because I respected it since this layer is about Bundler compatibility.

To try to clarify how deployment works with the other settings, the default path where Bundler installs gem is, by order of priority:

1. What's configured as path locally in ./.bundle/config.
2. What's set by BUNDLE_PATH if path not configured locally in ./.bundle/config.
3. What's configured as path globally in ~/.bundle/config if not configured locally nor in ENV.
4. vendor/bundle if deployment configured at any level and path not configured at any level.
5. Otherwise, the default path.

[deivid-rodriguez]

Other comment where I think this differs from my initial implementation is that I intentionally tried to make this "best effort" only and never fail if we can't read Bundler settings for whatever reason, just ignore them. We should definitely be debug-logging any failures, but I'm not 100% sure if we should fail if errors happen, since after all, it's not really required for rv to work.

[a-chacon]

@deivid-rodriguez I get it. I think this accomplishes the logic now. With default do you means return None at the path() function yes?.

You didn't mention the BUNDLE_PATH__SYSTEM in the flow. But based on the old code this just return none if it is present and true. This is respected too.

Also, I added a log and load default if something fails inside the bundler settings. Well let me know any comment on it.

[deivid-rodriguez]

@deivid-rodriguez I get it. I think this accomplishes the logic now. With default do you means return None at the path() function yes?.

Yes, because that means we end up setting GEM_HOME to the default path 👍

You didn't mention the BUNDLE_PATH__SYSTEM in the flow. But based on the old code this just return none if it is present and true. This is respected too.

True, I forgot that one. That's essentially an special value for BUNDLE_PATH that means "install to the default GEM_HOME". So yeah, you got that one right. If true, it means return None so the code I linked to above fallbacks to the default GEM_HOME 👍

Also, I added a log and load default if something fails inside the bundler settings. Well let me know any comment on it.

Cool, I'll review this later today, thank you so much!

[deivid-rodriguez]

Looking great, just a small comment. I did notice #646 while reviewing, but not related to this PR. Needs a rebase though!

[deivid-rodriguez]

Thank you @a-chacon! Just did some trivial squashing of dummy review commits and I'm now merging this 💪