Don't error out if a bucket is configured for a non-capable guest

[tmatilai]

Currently if you explicitly enable a bucket and the guest does not support it, a RuntimeError is raised. I would like to globally have auto_detect turned off (as I use a caching proxy for apt), but enable yum and gem caches. But then the yum bucket gets angry on debianoid boxes. :/

IMHO a warning message would be enough in these cases. I can make a PR if you agree.

If the consensus is against it, at least the raised error should at least be a subclass of Vagrant::Errors::VagrantError (see "ERROR HANDLING" in the docs). And I would like to have something like this instead:

config.cache.enable :yum, auto_detect: true

[thedrow]

+1 on using VagrantError. I'm really not sure if it should turn to a warning instead of an error.

[tmatilai]

My reasoning for downgrading to warning is that you most probably want to configure the caches globally in ~/.vagrant.d/Vagrantfile. But there is no way to make logic based on the guest type (other that maybe parsing the box name, but that is fragile). And that is what guest capabilities are for.

What is the issue that the error is trying to protect?

[fgrehm]

@tmatilai the idea behind throwing those errors is basically to make sure that users have their caching configs set properly. I think it wouldn't be a problem to configure a pacman cache for ubuntu machines, but it just doesn't feel right to me.

So back to your problem, if I got it right you want to enable automatic bucket detection but you want to prevent it from using the apt bucket right? I actually think that is a nice feature to have and I can think of a few different ways of doing it from the Vagrantfile apart from the one you mentioned:

Vagrant.configure("2") do |config|
  # option 1
  config.cache.auto_detect    = true
  config.cache.skip_detection = [:apt] # Need to think of a better name

  # option 2
  config.cache.auto_detect :all, except: [:apt]

  # option 3
  config.cache.auto_detect = [:yum, :gem, :rvm]
end

I haven't had the need to have something like that so far, so it's really up to you guys to decide which approach makes more sense. If you guys can think of something else just LMK :)

regarding raising Vagrant::Errors::VagrantErrors, I'm aware of that :) It's just not my top priority since I basically only use auto bucket detection and those errors only blows up in case you explicit define a bucket for a machine that doesn't support it, but I'd love to look into a PR with that change :)

[tmatilai]

@fgrehm thanks for the reply!

I think it wouldn't be a problem to configure a pacman cache for ubuntu machines, but it just doesn't feel right to me.

I didn't suggest that. I just want to configure the plugin globally, in a way that it works for all types of guests. I tried to propose that instead of raising an error (and thus failing the whole Vagrant run) we show an error message and do nothing with that bucket.

[fgrehm]

@tmatilai my bad then :) what do u think about the options I gave? I just think warnings are pretty annoying :P

[tmatilai]

Heh, I think the current stack trace is even more annoying. :P In a similar case in vagrant-proxyconf I chose to print only an info level message. Would that be better? ;)

From your options I like the third one to have cleanest syntax. But no strong feelings with any of them.

[fgrehm]

@tmatilai yeah, an info level message would be nice and if you don't mind seeing it all the time I don't mind changing too :P feel free to submit a PR and join the crew :) so, if we just print an info message in that case, there is no urge in implementing support for fine grained auto detection right? we can do that later on if someone else has a need for it ;)

[tmatilai]

@fgrehm I would be perfectly happy with just the info message and wouldn't need the other options. Sure I can do a PR but it will have to wait at least a couple of days as today's OSS window is closing and the weekend passes traveling.

[tmatilai]

@fgrehm OK, here you have finally the PR.

[fgrehm]

@tmatilai awesome! tks a lot :) I'll look into merging this over the weekend

[patcon]

:+1: to premise (not tested)

One thought: Could we make the UI message dynamic based on bucket class name? Would make it that much simpler to move this later and DRY out buckets :)

[fgrehm]

@patcon yup, I'm planning to do some house cleaning over the weekend (and introduce some testing as well :)

[tmatilai]

Yeah there is a lot of duplicated code in the Bucket classes, but the refactoring is IMHO out of scope for this issue.

[fgrehm]

@tmatilai sure, I'll just merge this one and continue from there :)

[fgrehm]

Merged!

@tmatilai welcome aboard :)

[tmatilai]

Thanks! All I miss is some extra time. ;)

[fgrehm]

@tmatilai don't worry, some :thumbsup:s or sharing your opinion on the things you see around will be more than welcome :)