search

desc / puppet-reprepro

Handle local repositories of debian packages
21 stars 25 forks source link

distribution not_automatic should not default to yes #35

Open gaudenz opened 9 years ago

gaudenz commented 9 years ago

Having a distribution set to not_automatic by default seems like a weird default. This means that all distributions are by default setup in a way that apt and aptitude won't install packages from them and not even update packages from there. This is the same setup as used by the Debian experimental suite.

This seems like a very special setup to me which should not be the default. And the not_automatic parameter should be a boolean instead of a pseudo boolean yes/no. If you agree with this bug report I can provide a patch and pull request.

jtopjian commented 9 years ago

I agree with you. As best as I can tell, this setting has always been set to yes and just overlooked.

I imagine the reason to set it to yes was to provide conservative defaults: "it's better to ask permission than forgiveness" type thing.

At this point, I think setting it to no might impact installations where this module is already deployed. How about changing it to the proper boolean value and a doc patch explaining when and why this should be set to no?

gaudenz commented 9 years ago

I agree that there is some value in not breaking backwards compatibility. But changeing this to a boolean will break compatibility for everyone (a majority?) how currently changes this parameter. So why not just also change the default at the same time? And document the backwards incompatible change in the documentation.

jtopjian commented 9 years ago

Right, so there's two different changes here:

  1. Changing the type of a parameter value
  2. Changing the default setting of a parameter value

The second change is what I'm most concerned about. I want to ensure that the group of users who did not explicitly set this parameter are not affected by this change, no matter how small that group may be. If this module ever goes through a serious rewrite, I think that'd be the time to make changes like this.

With regard to the group of users who have already set it to "no", the best thing might be to do a validate_bool check and fail if it is not boolean. This will alert the user early during the Puppet apply cycle that something needs changed. (as well as make a note in the README and stuff like that)